kgyrtkirk commented on a change in pull request #2865:
URL: https://github.com/apache/hive/pull/2865#discussion_r769679549
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/Driver.java
##########
@@ -515,14 +516,16 @@ private void preparForCompile(boolean resetTaskIds)
throws CommandProcessorExcep
}
private void prepareContext() throws CommandProcessorException {
+ String originalCboInfo = context != null ? context.cboInfo : null;
Review comment:
I feel that this save/load is a bit out-of-scope here;
I think that it might be possible to move this into the
`ReCompilePlugin#Hook` - what do you think?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
##########
@@ -683,20 +681,17 @@ Operator genOPTree(ASTNode ast, PlannerContext
plannerCtx) throws SemanticExcept
// Wrap all other errors (Should only hit in tests)
throw new SemanticException(e);
} else {
- reAnalyzeAST = true;
+ String strategies =
conf.getVar(ConfVars.HIVE_QUERY_REEXECUTION_STRATEGIES);
Review comment:
I think it would be more readable what/why are you doing this if there
would be a method like move this into some method like
`ReCompilationPlugin#isEnabled(conf)` or something like that
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSemanticAnalyzer.java
##########
@@ -156,7 +157,11 @@ public void analyzeInternal(ASTNode ast) throws
SemanticException {
while (driver.getResults(new ArrayList<String>())) {
}
} catch (CommandProcessorException e) {
- throw new SemanticException(e.getMessage(), e);
+ if (e.getCause() instanceof ReCompileException) {
Review comment:
I think right now we don't have any scenario in which this if would
become true; this excepiont will only be thrown when it will be handled
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
##########
@@ -683,20 +681,17 @@ Operator genOPTree(ASTNode ast, PlannerContext
plannerCtx) throws SemanticExcept
// Wrap all other errors (Should only hit in tests)
throw new SemanticException(e);
} else {
- reAnalyzeAST = true;
+ String strategies =
conf.getVar(ConfVars.HIVE_QUERY_REEXECUTION_STRATEGIES);
+ if (strategies == null ||
!Arrays.stream(strategies.split(",")).anyMatch("recompile_without_cbo"::equals))
{
+ throw new SemanticException("Invalid configuration. If
fallbackStrategy is set to " + fallbackStrategy.name() +
+ " then " +
ConfVars.HIVE_QUERY_REEXECUTION_STRATEGIES.varname + " should contain
'recompile_without_cbo'");
Review comment:
if this is a sanity check then I think this exception should be thrown
all the time regardless we are trying to fall back or not.
Imagine the following:
almost all queries are handled by CBO; and then comes 1 query and they hit
this exception....instead of seeing what the issue is they will start
investigating this exception....
you could move this check somewhere into `ReCompilePlugin` - and call that
this check when the plugin is not initialized - I think that would be a better
place to throw this exception; or not throw it at all.
...or we could add the `ReCompilePlugin` based on the actual setting of the
`fallbackStrategy`; but in that case this will became a secret
contract...(there's always something on the flipside :D)
...but automating things is also problematic; because if we add it
automatically shouldn't we also throw an exception if the reexecution is
disabled alltogether?
##########
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:
keep the latest changeset
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/reexec/ReExecutionCBOPlugin.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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;
+
+/**
+ * Re-compiles the query without CBO
+ */
+public class ReExecutionCBOPlugin implements IReExecutionPlugin {
+
+ private Driver driver;
+ private boolean retryPossible;
+ private String cboMsg;
+
+ 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();
+ retryPossible = throwable != null && throwable instanceof
ReCompileException;
Review comment:
I think it would be good to have log message about this decision
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
