[ https://issues.apache.org/jira/browse/HIVE-25792?focusedWorklogId=694913&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-694913 ]
ASF GitHub Bot logged work on HIVE-25792: ----------------------------------------- Author: ASF GitHub Bot Created on: 13/Dec/21 09:07 Start Date: 13/Dec/21 09:07 Worklog Time Spent: 10m Work Description: kgyrtkirk commented on a change in pull request #2865: URL: https://github.com/apache/hive/pull/2865#discussion_r767488415 ########## File path: ql/src/java/org/apache/hadoop/hive/ql/DriverUtils.java ########## @@ -171,4 +174,24 @@ public static String getUserFromUGI(DriverContext driverContext) throws CommandP throw createProcessorException(driverContext, 10, errorMessage, ErrorMsg.findSQLState(e.getMessage()), e); } } + + public static HookContext getHookContext(DriverContext driverContext, Context context) { Review comment: looks like a contructor to me ########## File path: ql/src/java/org/apache/hadoop/hive/ql/HookRunner.java ########## @@ -121,19 +121,27 @@ void runBeforeCompileHook(String command) { } /** - * Dispatches {@link QueryLifeTimeHook#afterCompile(QueryLifeTimeHookContext, boolean)}. - * - * @param command the Hive command that is being run - * @param compileError true if there was an error while compiling the command, false otherwise - */ - void runAfterCompilationHook(String command, boolean compileError) { + * Dispatches {@link QueryLifeTimeHook#afterCompile(QueryLifeTimeHookContext, boolean)}. + * + * @param driverContext the DriverContext used for generating the HookContext + * @param analyzerContext the SemanticAnalyzer context for this query + * @param compileException the exception if one was thrown during the compilation + */ + void runAfterCompilationHook(DriverContext driverContext, Context analyzerContext, Throwable compileException) { Review comment: this looks off....why do we have to pass 2 Context objects - its really the case that one of them know about the other? ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java ########## @@ -190,52 +229,21 @@ public CommandProcessorResponse run() throws CommandProcessorException { } PlanMapper newPlanMapper = coreDriver.getPlanMapper(); - if (!explainReOptimization && !shouldReExecuteAfterCompile(oldPlanMapper, newPlanMapper)) { + if (!explainReOptimization && + !plugins.stream().anyMatch(p -> p.shouldReExecute(executionIndex, oldPlanMapper, newPlanMapper))) { LOG.info("re-running the query would probably not yield better results; returning with last error"); // FIXME: retain old error; or create a new one? return cpr; } } } - private void afterExecute(PlanMapper planMapper, boolean success) { - for (IReExecutionPlugin p : plugins) { - p.afterExecute(planMapper, success); - } - } - - private boolean shouldReExecuteAfterCompile(PlanMapper oldPlanMapper, PlanMapper newPlanMapper) { - boolean ret = false; - for (IReExecutionPlugin p : plugins) { - boolean shouldReExecute = p.shouldReExecute(executionIndex, oldPlanMapper, newPlanMapper); - LOG.debug("{}.shouldReExecuteAfterCompile = {}", p, shouldReExecute); Review comment: undo this change as the old method also provided more details - and is also more debugger friendly ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java ########## @@ -190,52 +229,21 @@ public CommandProcessorResponse run() throws CommandProcessorException { } PlanMapper newPlanMapper = coreDriver.getPlanMapper(); - if (!explainReOptimization && !shouldReExecuteAfterCompile(oldPlanMapper, newPlanMapper)) { + if (!explainReOptimization && + !plugins.stream().anyMatch(p -> p.shouldReExecute(executionIndex, oldPlanMapper, newPlanMapper))) { LOG.info("re-running the query would probably not yield better results; returning with last error"); // FIXME: retain old error; or create a new one? return cpr; } } } - private void afterExecute(PlanMapper planMapper, boolean success) { - for (IReExecutionPlugin p : plugins) { - p.afterExecute(planMapper, success); - } - } - - private boolean shouldReExecuteAfterCompile(PlanMapper oldPlanMapper, PlanMapper newPlanMapper) { - boolean ret = false; - for (IReExecutionPlugin p : plugins) { - boolean shouldReExecute = p.shouldReExecute(executionIndex, oldPlanMapper, newPlanMapper); Review comment: rename `p.shouldReExecute` to `p.shouldReExecuteAfterCompile` ########## File path: ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/TestCBOReCompilation.java ########## @@ -0,0 +1,115 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hive.ql.optimizer.calcite; + +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hadoop.hive.conf.HiveConf.ConfVars; +import org.apache.hadoop.hive.ql.DriverFactory; +import org.apache.hadoop.hive.ql.IDriver; +import org.apache.hadoop.hive.ql.processors.CommandProcessorException; +import org.apache.hadoop.hive.ql.session.SessionState; +import org.apache.hive.testutils.HiveTestEnvSetup; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; + +public class TestCBOReCompilation { + + @ClassRule + public static HiveTestEnvSetup env_setup = new HiveTestEnvSetup(); + + @BeforeClass + public static void beforeClass() throws Exception { + try (IDriver driver = createDriver()) { + dropTables(driver); + String[] cmds = { + // @formatter:off + "create table aa1 ( stf_id string)", + "create table bb1 ( stf_id string)", + "create table cc1 ( stf_id string)", + "create table ff1 ( x string)" + // @formatter:on + }; + for (String cmd : cmds) { + driver.run(cmd); + } + } + } + + @AfterClass + public static void afterClass() throws Exception { + try (IDriver driver = createDriver()) { + dropTables(driver); + } + } + + public static void dropTables(IDriver driver) throws Exception { + String[] tables = new String[] {"aa1", "bb1", "cc1", "ff1" }; + for (String t : tables) { + driver.run("drop table if exists " + t); + } + } + + @Test + public void testReExecutedOnError() throws Exception { + try (IDriver driver = createDriver("ALWAYS")) { + String query = "explain from ff1 as a join cc1 as b " + + "insert overwrite table aa1 select stf_id GROUP BY b.stf_id " + + "insert overwrite table bb1 select b.stf_id GROUP BY b.stf_id"; + driver.run(query); + } + } + + @Test + public void testFailOnError() throws Exception { + try (IDriver driver = createDriver("TEST")) { + String query = "explain from ff1 as a join cc1 as b " + + "insert overwrite table aa1 select stf_id GROUP BY b.stf_id " + + "insert overwrite table bb1 select b.stf_id GROUP BY b.stf_id"; + Assert.assertThrows("Plan not optimized by CBO", CommandProcessorException.class, () -> driver.run(query)); Review comment: these are bad tests - because it kinda sets in stone that we should not fix this issue if we let this in - then you will force who will fix this issue to also invent a new query for this which: * fails in cbo * doesn not fail without cbo I don't think that's fair - so a different test is needed but I understand that this is something which should be handle....right now I don't have any good ideas what we could do instead of this - I'll keep thinking about it... ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java ########## @@ -167,20 +201,25 @@ public CommandProcessorResponse run() throws CommandProcessorException { } PlanMapper oldPlanMapper = coreDriver.getPlanMapper(); - afterExecute(oldPlanMapper, cpr != null); + final boolean success = cpr != null; + plugins.forEach(p -> p.afterExecute(oldPlanMapper, success)); + + // If the execution was successful return the result + if (success) { Review comment: you are missing the reexecutions of `explainReOptimization` ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/IReExecutionPlugin.java ########## @@ -42,24 +42,72 @@ /** * Called before executing the query. */ - void beforeExecute(int executionIndex, boolean explainReOptimization); + default void beforeExecute(int executionIndex, boolean explainReOptimization) { + // default noop + } /** * The query have failed, does this plugin advises to re-execute it again? */ - boolean shouldReExecute(int executionNum); + default boolean shouldReExecute(int executionNum) { Review comment: why do we have 2 `shouldReExecute` methods? ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java ########## @@ -167,20 +201,25 @@ public CommandProcessorResponse run() throws CommandProcessorException { } PlanMapper oldPlanMapper = coreDriver.getPlanMapper(); - afterExecute(oldPlanMapper, cpr != null); + final boolean success = cpr != null; + plugins.forEach(p -> p.afterExecute(oldPlanMapper, success)); + + // If the execution was successful return the result + if (success) { + return cpr; + } boolean shouldReExecute = explainReOptimization && executionIndex==1; - shouldReExecute |= cpr == null && shouldReExecute(); + shouldReExecute |= plugins.stream().anyMatch(p -> p.shouldReExecute(executionIndex)); - if (executionIndex >= maxExecutuions || !shouldReExecute) { - if (cpr != null) { - return cpr; - } else { - throw cpe; - } + if (executionIndex >= maxExecutions || !shouldReExecute) { + // If we do not have to reexecute, return the last error + throw cpe; } + LOG.info("Preparing to re-execute query"); - prepareToReExecute(); + plugins.forEach(IReExecutionPlugin::prepareToReExecute); Review comment: undo these changes; as the old method also provided more details ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java ########## @@ -190,52 +229,21 @@ public CommandProcessorResponse run() throws CommandProcessorException { } PlanMapper newPlanMapper = coreDriver.getPlanMapper(); - if (!explainReOptimization && !shouldReExecuteAfterCompile(oldPlanMapper, newPlanMapper)) { + if (!explainReOptimization && + !plugins.stream().anyMatch(p -> p.shouldReExecute(executionIndex, oldPlanMapper, newPlanMapper))) { LOG.info("re-running the query would probably not yield better results; returning with last error"); // FIXME: retain old error; or create a new one? return cpr; } } } - private void afterExecute(PlanMapper planMapper, boolean success) { - for (IReExecutionPlugin p : plugins) { - p.afterExecute(planMapper, success); - } - } - - private boolean shouldReExecuteAfterCompile(PlanMapper oldPlanMapper, PlanMapper newPlanMapper) { - boolean ret = false; - for (IReExecutionPlugin p : plugins) { - boolean shouldReExecute = p.shouldReExecute(executionIndex, oldPlanMapper, newPlanMapper); - LOG.debug("{}.shouldReExecuteAfterCompile = {}", p, shouldReExecute); - ret |= shouldReExecute; - } - return ret; - } - - private boolean shouldReExecute() { - boolean ret = false; - for (IReExecutionPlugin p : plugins) { - boolean shouldReExecute = p.shouldReExecute(executionIndex); - LOG.debug("{}.shouldReExecute = {}", p, shouldReExecute); Review comment: undo this change as the old method also provided more details ########## File path: ql/src/java/org/apache/hadoop/hive/ql/DriverUtils.java ########## @@ -171,4 +174,24 @@ public static String getUserFromUGI(DriverContext driverContext) throws CommandP throw createProcessorException(driverContext, 10, errorMessage, ErrorMsg.findSQLState(e.getMessage()), e); } } + + public static HookContext getHookContext(DriverContext driverContext, Context context) { + String host = "Unknown"; + try { + host = InetAddress.getLocalHost().getHostAddress(); + } catch (UnknownHostException e) { + LOG.warn("Unable to get host", e); Review comment: this is a fatal error - no need to mask it ########## File path: ql/src/java/org/apache/hadoop/hive/ql/DriverUtils.java ########## @@ -171,4 +174,24 @@ public static String getUserFromUGI(DriverContext driverContext) throws CommandP throw createProcessorException(driverContext, 10, errorMessage, ErrorMsg.findSQLState(e.getMessage()), e); } } + + public static HookContext getHookContext(DriverContext driverContext, Context context) { + String host = "Unknown"; + try { + host = InetAddress.getLocalHost().getHostAddress(); + } catch (UnknownHostException e) { + LOG.warn("Unable to get host", e); + } + + try { + return new PrivateHookContext(driverContext.getPlan(), driverContext.getQueryState(), + context.getPathToCS(), SessionState.get().getUserName(), SessionState.get().getUserIpAddress(), + host, driverContext.getOperationId(), + SessionState.get().getSessionId(), Thread.currentThread().getName(), SessionState.get().isHiveServerQuery(), + SessionState.getPerfLogger(), driverContext.getQueryInfo(), context); + } catch (Exception e) { + LOG.warn("Unable to create hook context"); + return null; Review comment: should be an exception - returning null will just make things worse if that happens ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java ########## @@ -115,14 +115,48 @@ public ReExecDriver(QueryState queryState, QueryInfo queryInfo, ArrayList<IReExe } } + // I think this should be used only in tests Review comment: I don't really understand why this method was even neccessary; but instead of the comment can we could probably use `@VisibleForTesting` ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecutionCBOPlugin.java ########## @@ -0,0 +1,97 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hive.ql.reexec; + +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hadoop.hive.ql.Driver; +import org.apache.hadoop.hive.ql.hooks.QueryLifeTimeHook; +import org.apache.hadoop.hive.ql.hooks.QueryLifeTimeHookContext; +import org.apache.hadoop.hive.ql.parse.CBOException; +import org.apache.hadoop.hive.ql.parse.SemanticException; + +/** + * Re-compiles the query without CBO + */ +public class ReExecutionCBOPlugin implements IReExecutionPlugin { + + private Driver driver; + private boolean retryPossible = false; + private CBOFallbackStrategy fallbackStrategy; + + class LocalHook implements QueryLifeTimeHook { + @Override + public void beforeCompile(QueryLifeTimeHookContext ctx) { + // noop + } + + @Override + public void afterCompile(QueryLifeTimeHookContext ctx, boolean hasError) { + if (hasError) { + Throwable throwable = ctx.getHookContext().getException(); + if (throwable != null) { + if (throwable instanceof CBOException) { + // Determine if we should re-throw the exception OR if we retry planning with non-CBO. + if (fallbackStrategy.isFatal((CBOException) throwable)) { + Throwable cause = throwable.getCause(); + if (cause instanceof RuntimeException || cause instanceof SemanticException) { + // These types of exceptions do not need wrapped + retryPossible = false; + return; + } + // Wrap all other errors (Should only hit in tests) + throw new RuntimeException(cause); + } else { + // Only if the exception is a CBOException then we can retry + retryPossible = true; Review comment: remove all other stuff and retain only the path to this assignment; a log message would probably needed - as you are discarding the `CBOException` and retrying the query. ########## File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java ########## @@ -5536,10 +5536,12 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal HIVE_QUERY_REEXECUTION_ENABLED("hive.query.reexecution.enabled", true, "Enable query reexecutions"), - HIVE_QUERY_REEXECUTION_STRATEGIES("hive.query.reexecution.strategies", "overlay,reoptimize,reexecute_lost_am,dagsubmit", + HIVE_QUERY_REEXECUTION_STRATEGIES("hive.query.reexecution.strategies", + "overlay,reoptimize,reexecute_lost_am,dagsubmit,reexecute_cbo", "comma separated list of plugin can be used:\n" + " overlay: hiveconf subtree 'reexec.overlay' is used as an overlay in case of an execution errors out\n" + " reoptimize: collects operator statistics during execution and recompile the query after a failure\n" + + " reexecute_cbo: reexecutes query after a CBO failure\n" Review comment: I think this name is misleading; I don't have a good name for it but something like: * recompile without cbo * fallback to non-cbo path * non-cbo fallback ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecDriver.java ########## @@ -167,20 +201,25 @@ public CommandProcessorResponse run() throws CommandProcessorException { } PlanMapper oldPlanMapper = coreDriver.getPlanMapper(); - afterExecute(oldPlanMapper, cpr != null); + final boolean success = cpr != null; + plugins.forEach(p -> p.afterExecute(oldPlanMapper, success)); + + // If the execution was successful return the result + if (success) { + return cpr; + } boolean shouldReExecute = explainReOptimization && executionIndex==1; - shouldReExecute |= cpr == null && shouldReExecute(); + shouldReExecute |= plugins.stream().anyMatch(p -> p.shouldReExecute(executionIndex)); - if (executionIndex >= maxExecutuions || !shouldReExecute) { - if (cpr != null) { - return cpr; - } else { - throw cpe; - } + if (executionIndex >= maxExecutions || !shouldReExecute) { + // If we do not have to reexecute, return the last error + throw cpe; } + LOG.info("Preparing to re-execute query"); - prepareToReExecute(); + plugins.forEach(IReExecutionPlugin::prepareToReExecute); + try { coreDriver.compileAndRespond(currentQuery); Review comment: during a re-execution recompile are we ok to call `coreDriver` ? ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecutionCBOPlugin.java ########## @@ -0,0 +1,97 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hive.ql.reexec; + +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hadoop.hive.ql.Driver; +import org.apache.hadoop.hive.ql.hooks.QueryLifeTimeHook; +import org.apache.hadoop.hive.ql.hooks.QueryLifeTimeHookContext; +import org.apache.hadoop.hive.ql.parse.CBOException; +import org.apache.hadoop.hive.ql.parse.SemanticException; + +/** + * Re-compiles the query without CBO + */ +public class ReExecutionCBOPlugin implements IReExecutionPlugin { + + private Driver driver; + private boolean retryPossible = false; + private CBOFallbackStrategy fallbackStrategy; + + class LocalHook implements QueryLifeTimeHook { + @Override + public void beforeCompile(QueryLifeTimeHookContext ctx) { + // noop + } + + @Override + public void afterCompile(QueryLifeTimeHookContext ctx, boolean hasError) { + if (hasError) { Review comment: set `retryPossible` to some value here; instead of retaining the value of the previous run ########## File path: ql/src/java/org/apache/hadoop/hive/ql/reexec/IReExecutionPlugin.java ########## @@ -42,24 +42,72 @@ /** * Called before executing the query. */ - void beforeExecute(int executionIndex, boolean explainReOptimization); + default void beforeExecute(int executionIndex, boolean explainReOptimization) { + // default noop + } /** * The query have failed, does this plugin advises to re-execute it again? */ - boolean shouldReExecute(int executionNum); + default boolean shouldReExecute(int executionNum) { + // default no + return false; + } /** - * The plugin should prepare for the re-compilaton of the query. + * The plugin should prepare for the re-compilation of the query. */ - void prepareToReExecute(); + default void prepareToReExecute() { + // default noop + } /** - * The query have failed; and have been recompiled - does this plugin advises to re-execute it again? + * The query has failed; and have been recompiled - does this plugin advises to re-execute it again? */ - boolean shouldReExecute(int executionNum, PlanMapper oldPlanMapper, PlanMapper newPlanMapper); + default boolean shouldReExecute(int executionNum, PlanMapper oldPlanMapper, PlanMapper newPlanMapper) { Review comment: returning with `false` at this point and not implementing this method will make an reexec plugin pratcially useless...so it's not an option to not implement it. please remove these default implementations for methods returning booleans; plugin implementors should answer these questions; -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org Issue Time Tracking ------------------- Worklog Id: (was: 694913) Time Spent: 0.5h (was: 20m) > Multi Insert query fails on CBO path > ------------------------------------- > > Key: HIVE-25792 > URL: https://issues.apache.org/jira/browse/HIVE-25792 > Project: Hive > Issue Type: Bug > Reporter: Zoltan Haindrich > Assignee: Peter Vary > Priority: Major > Labels: pull-request-available > Time Spent: 0.5h > Remaining Estimate: 0h > > {code} > set hive.cbo.enable=true; > drop table if exists aa1; > drop table if exists bb1; > drop table if exists cc1; > drop table if exists dd1; > drop table if exists ee1; > drop table if exists ff1; > create table aa1 ( stf_id string); > create table bb1 ( stf_id string); > create table cc1 ( stf_id string); > create table ff1 ( x string); > explain > from ff1 as a join cc1 as b > insert overwrite table aa1 select stf_id GROUP BY b.stf_id > insert overwrite table bb1 select b.stf_id GROUP BY b.stf_id > ; > {code} -- This message was sent by Atlassian Jira (v8.20.1#820001)