Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/17610 )
Change subject: IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int) ...................................................................... Patch Set 1: (4 comments) Added some comments. Overall the fix looks good to me. http://gerrit.cloudera.org:8080/#/c/17610/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17610/1//COMMIT_MSG@11 PS1, Line 11: expressions. This can lead to the generation of an abnormal plan Maybe also put one line about what the issue was in this case. So something like "If the Join order is flipped and the planner treats two same predicates (with different letter case) as different, we end up with a plan where each side of the JOIN references columns from the other side." http://gerrit.cloudera.org:8080/#/c/17610/1/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/17610/1/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1026 PS1, Line 1026: String targetConjuctSql = e.toSql().toLowerCase(); Would be good to add a comment as to why we are converting conjuncts to the lower case. Something like, "Convert conjuncts to lower case so that comparison is not case sensitive, else planner might treat the same conjuncts as different" http://gerrit.cloudera.org:8080/#/c/17610/1/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1028 PS1, Line 1028: if (targetConjuctSql.compareTo(bestConjuctSql) < 0) { Actually, might be better to use compareToIgnoreCase here instead? http://gerrit.cloudera.org:8080/#/c/17610/1/testdata/workloads/tpch/queries/tpch-outer-joins.test File testdata/workloads/tpch/queries/tpch-outer-joins.test: http://gerrit.cloudera.org:8080/#/c/17610/1/testdata/workloads/tpch/queries/tpch-outer-joins.test@50 PS1, Line 50: ---- QUERY We should also add a planner test for the same query. If the JOIN order doesn't flip then this query doesn't fail. Basically, the plan should flip the JOIN order from the original query and also reference the proper columns in the predicate. You could probably add new test here: https://github.com/apache/impala/blob/master/testdata/workloads/functional-planner/queries/PlannerTest/join-order.test -- To view, visit http://gerrit.cloudera.org:8080/17610 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2ba031d7a6eda21a77b0e53bc41772ee9e00a528 Gerrit-Change-Number: 17610 Gerrit-PatchSet: 1 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 25 Jun 2021 19:14:31 +0000 Gerrit-HasComments: Yes
