Yida Wu 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 2: (6 comments) 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: not the best and there is a same conjunct with different letter cases. > Maybe also put one line about what the issue was in this case. So something Done 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: bestConjunctIdx = i; > Would be good to add a comment as to why we are converting conjuncts to the Changed another way to solve the problem, which is direct cause that it removes the wrong conjuct Object in the remaining list when two conjuct are the same but with different letter cases, therefore, it can result in returning a list with two identical conjuct Objects after sorting, and being flipped twice later on the same Object. I guess in this way we don't need to consider the letter case to fix the bug, however I think we may need to file another Jira to review the letter case problem during the SQL analyzing. http://gerrit.cloudera.org:8080/#/c/17610/1/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1026 PS1, Line 1026: > If SQL identifiers (such as column names) are involved during comparison, I Changed to another way to fix the bug, but we may need to review the letter case issue during SQL analyzing. http://gerrit.cloudera.org:8080/#/c/17610/1/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1028 PS1, Line 1028: // Break ties based on toSql() to get a consistent display in explain plans. > Actually, might be better to use compareToIgnoreCase here instead? Changed another way to solve the problem. 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 doe Done http://gerrit.cloudera.org:8080/#/c/17610/1/testdata/workloads/tpch/queries/tpch-outer-joins.test@55 PS1, Line 55: c.c_custkey = l.l_orderkey and c.C_CUSTKEY = l.L_ORDERKEY > haven't looked into details, but do you know why these two identical predic looks it needs some optimization in the analyzer, but the case should happen rarely, I guess I can file a Jira to do the de-duplicate as an improvement later. -- 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: 2 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Fri, 09 Jul 2021 04:43:42 +0000 Gerrit-HasComments: Yes
