[hive] branch master updated: HIVE-23106: Cleanup CalcitePlanner genOPTree exception handling (John Sherman, reviewed by Jesus Camacho Rodriguez)

[jc...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

jcamacho pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hive.git


The following commit(s) were added to refs/heads/master by this push:
     new 4e936c4  HIVE-23106: Cleanup CalcitePlanner genOPTree exception handling (John Sherman, reviewed by Jesus Camacho Rodriguez)
4e936c4 is described below

commit 4e936c4ce17ebf19b551d0abbc3472fc62e119ac
Author: John Sherman <jf...@cloudera.com>
AuthorDate: Thu Apr 2 11:55:37 2020 -0700

    HIVE-23106: Cleanup CalcitePlanner genOPTree exception handling (John Sherman, reviewed by Jesus Camacho Rodriguez)
---
 .../hadoop/hive/ql/parse/CalcitePlanner.java       | 68 ++++++++++------------
 1 file changed, 30 insertions(+), 38 deletions(-)

diff --git a/ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java b/ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
index 9c39cb2..ee3aaa5 100644
--- a/ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
+++ b/ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
@@ -615,51 +615,43 @@ public class CalcitePlanner extends SemanticAnalyzer {
           }
         } catch (Exception e) {
           LOG.error("CBO failed, skipping CBO. ", e);
+
+          String cboMsg = "Plan not optimized by CBO.";
           boolean isMissingStats = noColsMissingStats.get() > 0;
           if (isMissingStats) {
             LOG.error("CBO failed due to missing column stats (see previous errors), skipping CBO");
-            this.ctx
-                .setCboInfo("Plan not optimized by CBO due to missing statistics. Please check log for more details.");
-          } else {
-            if (e instanceof CalciteSemanticException) {
-              CalciteSemanticException calciteSemanticException = (CalciteSemanticException) e;
-              UnsupportedFeature unsupportedFeature = calciteSemanticException
-                  .getUnsupportedFeature();
-              if (unsupportedFeature != null) {
-                this.ctx.setCboInfo("Plan not optimized by CBO due to missing feature ["
-                    + unsupportedFeature + "].");
-              } else {
-                this.ctx.setCboInfo("Plan not optimized by CBO.");
-              }
-            } else {
-              this.ctx.setCboInfo("Plan not optimized by CBO.");
+            cboMsg = "Plan not optimized by CBO due to missing statistics. Please check log for more details.";
+          } else if (e instanceof CalciteSemanticException) {
+            CalciteSemanticException cse = (CalciteSemanticException) e;
+            UnsupportedFeature unsupportedFeature = cse.getUnsupportedFeature();
+            if (unsupportedFeature != null) {
+              cboMsg = "Plan not optimized by CBO due to missing feature [" + unsupportedFeature + "].";
             }
           }
-          if( e instanceof CalciteSubquerySemanticException
-                || e instanceof CalciteSubqueryRuntimeException) {
-            // non-cbo path retries to execute subqueries and throws completely different exception/error
-            // to eclipse the original error message
-            // so avoid executing subqueries on non-cbo
+          this.ctx.setCboInfo(cboMsg);
+
+          // Determine if we should re-throw the exception OR if we try to mark plan as reAnayzeAST to retry
+          // planning as non-CBO.
+          if (e instanceof CalciteSubquerySemanticException || e instanceof CalciteViewSemanticException
+              || e instanceof CalciteSubqueryRuntimeException) {
+            // Non-CBO path for CalciteSubquerySemanticException fails with completely different error
+            // and masks the original failure.
+            // Non-CBO path for CalciteViewSemanticException would fail in a similar way as CBO path.
             throw new SemanticException(e);
           }
-          else if( e instanceof CalciteViewSemanticException) {
-            // non-cbo path retries to execute create view and
-            // we believe it will throw the same error message
-            throw new SemanticException(e);
-          }
-          else if (!conf.getBoolVar(ConfVars.HIVE_IN_TEST) || isMissingStats
-              || e instanceof CalciteSemanticException ) {
-              reAnalyzeAST = true;
-          } else if (e instanceof SemanticException && !conf.getBoolVar(ConfVars.HIVE_IN_TEST)) {
-            // although, its likely to be a valid exception, we will retry
-            // with cbo off anyway.
-            // for tests we would like to avoid retrying to catch cbo failures
-              reAnalyzeAST = true;
-          } else if (e instanceof RuntimeException) {
-            throw (RuntimeException) e;
-          } else if (e instanceof SemanticException) {
-            throw e;
-          } else {
+
+          boolean isHiveTest = conf.getBoolVar(ConfVars.HIVE_IN_TEST);
+          // At this point we retry with CBO off:
+          // 1) If this is not test mode (common case)
+          // 2) If we are in test mode and we are missing stats
+          // 3) if we are in test mode and a CalciteSemanticException is generated
+          reAnalyzeAST = (!isHiveTest || isMissingStats ||  e instanceof CalciteSemanticException);
+          if (!reAnalyzeAST) {
+            if (e instanceof RuntimeException || e instanceof SemanticException) {
+              // These types of exceptions do not need wrapped
+              throw e;
+            }
+            // Wrap all other errors (Should only hit in tests)
             throw new SemanticException(e);
           }
         } finally {