Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21679 )
Change subject: IMPALA-13302: Analyze new rewrite exprs ...................................................................... Patch Set 5: (2 comments) Thanks for digging into this! I'm still trying to understand why the conjuncts with Falsey conjuct in the same list will be considered again in the reAnalyze phase. Intuitively they should be removed. So it seems skiping registering them is not enough to remove/ignore them. http://gerrit.cloudera.org:8080/#/c/21679/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/21679/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3348 PS5, Line 3348: If we're marking it now, that can cause conflicts : // with an Expr that gets assigned the same ID, usually skipping slot materialization. It seems valid to have different Exprs using the same ExprId, this happens when they are not registered in the same GlobalState. When analyzing the WITH clause, we create a new GlobalState: https://github.com/apache/impala/blob/d91d99c08e469b4cd40d81ce1aeb8c2bec596880/fe/src/main/java/org/apache/impala/analysis/WithClause.java#L77 E.g. in the query reported in the JIRA: WITH v AS (SELECT 1 FROM functional.alltypestiny t1 JOIN functional.alltypestiny t2 ON t1.id = t2.id) SELECT 1 FROM functional.alltypestiny t1 WHERE ((t1.id = 1 and false) or (t1.id = 1 and false)) AND t1.id = 1 AND t1.id = 1 UNION ... The BinaryPredicate "t1.id = t2.id" in the WITH clause is assigned with ExprId=0. The conjunct "((t1.id = 1 and false) or (t1.id = 1 and false))" in the WHERE clause is also assigned with ExprId=0. http://gerrit.cloudera.org:8080/#/c/21679/5/fe/src/main/java/org/apache/impala/analysis/SlotRef.java File fe/src/main/java/org/apache/impala/analysis/SlotRef.java: http://gerrit.cloudera.org:8080/#/c/21679/5/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@332 PS5, Line 332: "Illegal reference to non-materialized slot: tid=%s sid=%s", This error message is not that helpful. Should we consider dumping the toString() result? It's ok to consider this in a future patch. -- To view, visit http://gerrit.cloudera.org:8080/21679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6be731c2ea79c96e51d199c822e2cb34e5bb3028 Gerrit-Change-Number: 21679 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 23 Aug 2024 08:22:06 +0000 Gerrit-HasComments: Yes
