[impala] 01/02: IMPALA-11021: Fix bug when query contains illegal predicate hints

[st...@apache.org]

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

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

commit 6f80a1f16015b628a31548bdbecd61a554fc0d85
Author: skyyws <sk...@163.com>
AuthorDate: Fri Nov 19 10:27:06 2021 +0800

    IMPALA-11021: Fix bug when query contains illegal predicate hints
    
    Currently Impala support predicate hint: ALWAYS_TRUE, we can use this
    hint after where keyword. If we use illegal hints carelessly, query
    will throw IllegalStateException which is not expected. Query should
    return normal results with a warning instead of a exception. This is
    due to the condition check in Analyzer.addWarning().
    After create TExecRequest and initialize it, Impala will get warnings
    from 'GlobalState.warnings', and 'GlobalState.warningsRetrieved' will
    be set to 'true' then. But after this, Impala will substitute predicate
    by clone(), and analyze new predicate in later phase. New predicate
    analyze will add hint warning to 'GlobalState.warnings', but failed and
    throw IllegalStateException due to 'globalState_.warningsRetrieved'
    check failed which is expected as 'false'.
    This check is added in IMPALA-4166, I removed original condition check
    and added a new check to ensure that all warnings for new/substituted
    predicates are already exists in 'globalState_.warnings'. And this will
    also avoiding exception caused by illegal hints.
    
    Testing:
    - Added new fe tests in 'AnalyzeStmtsTest'
    
    Change-Id: Id719bc4280c811456333eb4b4ec5bc9cb8bae128
    Reviewed-on: http://gerrit.cloudera.org:8080/18040
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 .../java/org/apache/impala/analysis/Analyzer.java  | 17 ++++++++++-
 .../apache/impala/analysis/AnalyzeStmtsTest.java   | 35 ++++++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
index 3b95bcd..6b5d137 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
@@ -3324,7 +3324,8 @@ public class Analyzer {
    * getWarnings() has been called, no warning may be added to the Analyzer anymore.
    */
   public void addWarning(String msg) {
-    Preconditions.checkState(!globalState_.warningsRetrieved);
+    if (checkWarningsRetrieved(msg)) return;
+
     if (msg == null) return;
     Integer count = globalState_.warnings.get(msg);
     if (count == null) count = 0;
@@ -3332,6 +3333,20 @@ public class Analyzer {
   }
 
   /**
+   * 'addWarning' method may be called after the warnings are retrieved, e.g. in
+   * analyzing some substituted/cloned predicates (IMPALA-11021). We need to make sure
+   * no new warnings are added after retrieved.
+   */
+  private boolean checkWarningsRetrieved(String msg) {
+    if (globalState_.warningsRetrieved) {
+      // Make sure that substituted/cloned predicates' warnings already exists in map.
+      Preconditions.checkState(globalState_.warnings.containsKey(msg));
+      return true;
+    }
+    return false;
+  }
+
+  /**
    * Registers a new PrivilegeRequest in the analyzer.
    */
   public void registerPrivReq(PrivilegeRequest privReq) {
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
index 5c38d37..f0c31e2 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
@@ -4966,4 +4966,39 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
         tblProperties,
         "Only BUCKET and TRUNCATE partition transforms accept a parameter.");
   }
+
+  @Test
+  public void testPredicateHint() {
+    // Legal hint return correct results without warning, we currently only support
+    // 'ALWAYS_TRUE' hint.
+    AnalyzesOk("select * from tpch.lineitem where /* +ALWAYS_TRUE */ " +
+            "l_shipdate <= (select '1998-09-02')");
+
+    // Illegal hint return correct results with a warning.
+    AnalyzesOk("select * from tpch.lineitem where /* +ALWAYS_TRUE_TEST */ " +
+            "l_shipdate <= (select '1998-09-02')",
+        "Predicate hint not recognized: ALWAYS_TRUE_TEST");
+    AnalyzesOk("select * from tpch.lineitem where /* +ILLEGAL_HINT_TEST */ " +
+            "l_shipdate <= (select '1998-09-02')",
+        "Predicate hint not recognized: ILLEGAL_HINT_TEST");
+
+    // Multiple hints with legal and illegal hints also output a warning.
+    AnalyzesOk("select * from tpch.lineitem where /* +ALWAYS_TRUE,ILLEGAL_HINT_TEST */ "
+            + "l_shipdate <= (select '1998-09-02')",
+        "Predicate hint not recognized: ILLEGAL_HINT_TEST");
+
+    // Multiple illegal hints will output each hint warnings.
+    AnalyzesOk("select * from tpch.lineitem where " +
+            "/* +ILLEGAL_HINT_TEST1,ILLEGAL_HINT_TEST2,ILLEGAL_HINT_TEST3 */ " +
+            "l_shipdate <= (select '1998-09-02')",
+        "Predicate hint not recognized: ILLEGAL_HINT_TEST1");
+    AnalyzesOk("select * from tpch.lineitem where " +
+            "/* +ILLEGAL_HINT_TEST1,ILLEGAL_HINT_TEST2,ILLEGAL_HINT_TEST3 */ " +
+            "l_shipdate <= (select '1998-09-02')",
+        "Predicate hint not recognized: ILLEGAL_HINT_TEST2");
+    AnalyzesOk("select * from tpch.lineitem where " +
+            "/* +ILLEGAL_HINT_TEST1,ILLEGAL_HINT_TEST2,ILLEGAL_HINT_TEST3 */ " +
+            "l_shipdate <= (select '1998-09-02')",
+        "Predicate hint not recognized: ILLEGAL_HINT_TEST3");
+  }
 }