Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/18579 )
Change subject: IMPALA-11323: Do not infer predicates on equivalent values ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/18579/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18579/2//COMMIT_MSG@7 PS2, Line 7: Do not infer predicates on equivalent values nit: We are still creating the inferred predicate (which is why isInferred() for the binary predicate returns true) but we are not going to evaluate it as part of the SELECT node. You could say 'Don't evaluate constants-only inferred predicates' or something similar. http://gerrit.cloudera.org:8080/#/c/18579/2//COMMIT_MSG@9 PS2, Line 9: IMPALA-10812 This is not the right jira id http://gerrit.cloudera.org:8080/#/c/18579/2/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/18579/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@1645 PS2, Line 1645: * Returns the source expression for this expression. nit: could you fix the alignment at the end of these 2 lines. http://gerrit.cloudera.org:8080/#/c/18579/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@1660 PS2, Line 1660: if (slotDesc.isScanSlot()) { nit: Impala code style put trivial single statement blocks in the same line i.e if (slotDesc.isScanSlot()) return slotRef; http://gerrit.cloudera.org:8080/#/c/18579/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@1677 PS2, Line 1677: public SlotDescriptor findSrcScanSlot() { Is findSrcScanSlot() getting used anywhere else after this change ? http://gerrit.cloudera.org:8080/#/c/18579/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/18579/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@588 PS2, Line 588: SlotRef lhsSlotRef = lhsSrcExpr.unwrapSlotRef(false); nit: it seems that unwrap is getting called multiple times ..once inside findSrcExpr and then here. Could you add comments here about the reason. http://gerrit.cloudera.org:8080/#/c/18579/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@592 PS2, Line 592: if (lhsSlotRef.getDesc() != null && A slot ref would always have non-null slot descriptor. http://gerrit.cloudera.org:8080/#/c/18579/2/testdata/workloads/functional-query/queries/QueryTest/inline-view.test File testdata/workloads/functional-query/queries/QueryTest/inline-view.test: http://gerrit.cloudera.org:8080/#/c/18579/2/testdata/workloads/functional-query/queries/QueryTest/inline-view.test@585 PS2, Line 585: with t as (select 1 a), v as nit: remove whitespace at the end of this and lines below -- To view, visit http://gerrit.cloudera.org:8080/18579 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If1cd4559dda406d2d38703ed594b70b41ed336fd Gerrit-Change-Number: 18579 Gerrit-PatchSet: 2 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 03 Jun 2022 08:25:00 +0000 Gerrit-HasComments: Yes
