Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19937 )
Change subject: IMPALA-12164: Filter out non-materialized slots in analytic limit pushdown ...................................................................... Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/19937/6/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java File fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java: http://gerrit.cloudera.org:8080/#/c/19937/6/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@155 PS6, Line 155: if (e instanceof BinaryPredicate) { What about other kinds of predicates? It seems not just BinaryPredicate can hit the bug. E.g. I change the predicate to be an InPredicate and it also fails: select id, RANK() OVER(ORDER BY id DESC) AS rank_id from (SELECT id FROM functional.alltypesagg WHERE int_col in (0, 1) and false) alias_0 order by id http://gerrit.cloudera.org:8080/#/c/19937/6/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@159 PS6, Line 159: if (rhs != null && !rhs.isMaterialized()) continue; I understand that in this case, the slot is unmaterialized since we don't need to evaluate the conjunct due to short-circuit evaluation on the constant FALSE conjunct. But it might be a bug at somewhere that we don't mark the slot as materialized for a conjunct that we can't skip. I think the inconsistency happens when we return the conjunct in getUnassignedConjuncts(). If we don't need to evaluate it, we should not consider it anymore. We evaluate the constant conjunct here: https://github.com/apache/impala/blob/ff3d0c79841b78f99fa78d81b1dca4371cd7bc99/fe/src/main/java/org/apache/impala/analysis/Analyzer.java#L1922 I think we should make markConstantConjunct() return boolean values to indicate whether it's constant FALSE. For constant FALSE conjunct, the other conjuncts in the same list should be marked as assigned so they won't be considered anymore. https://github.com/apache/impala/blob/ff3d0c79841b78f99fa78d81b1dca4371cd7bc99/fe/src/main/java/org/apache/impala/analysis/Analyzer.java#L1893 -- To view, visit http://gerrit.cloudera.org:8080/19937 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e078f48863c38062e1e624a1ff3e9317092466f Gerrit-Change-Number: 19937 Gerrit-PatchSet: 6 Gerrit-Owner: ttttttz <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: ttttttz <[email protected]> Gerrit-Comment-Date: Wed, 31 May 2023 07:15:33 +0000 Gerrit-HasComments: Yes
