You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Yida Wu (Code Review)" <ge...@cloudera.org> on 2021/06/21 17:05:18 UTC
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Yida Wu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17610
Change subject: IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
......................................................................
IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
The root problem of the issue is that some parts of the analyzer is
case-sensitive, which is not correct, while comparing with
expressions. This can lead to the generation of an abnormal plan
when the sql contains the same identity column with different
letter cases, and therefore crashes.
The patch fixes a bug in the function of orderConjunctsByCost, which
compares the conjunct sql using different letter cases and may lead
to a wrong cost calculation only because the different letter cases.
The fix is to change the SQLs to lower cases before the comparison,
so that if two identities are with different cases originally, they
will be considered the same. Some results of the testcases need to be
changed because of the fix, mainly the order adjustments, and supposed
not to bring any issues related to the function or correctness.
A regression testcase has been added to tpch-outer-joins.
Tests:
Ran the Core FE_TEST and EE_TEST.
Passed the regression test in tpch-outer-joins.
Change-Id: I2ba031d7a6eda21a77b0e53bc41772ee9e00a528
---
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test
M testdata/workloads/tpch/queries/tpch-outer-joins.test
3 files changed, 22 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/17610/1
--
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: newchange
Gerrit-Change-Id: I2ba031d7a6eda21a77b0e53bc41772ee9e00a528
Gerrit-Change-Number: 17610
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7324/ DRY_RUN=false
--
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: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Jul 2021 16:03:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen 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 5:
The verification run's failure is due to "ERROR: cannot verify jenkins.impala.io's certificate, issued by ‘CN=Go Daddy Secure Certificate Authority - G2,OU=http://certs.godaddy.com/repository/,O=GoDaddy.com\\, Inc.,L=Scottsdale,ST=Arizona,C=US’:".
Perhaps it is transient. I will start a new one.
--
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: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 13:05:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6: Code-Review+2
--
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: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 13:06:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7324/
--
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: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Jul 2021 22:20:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9055/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 09 Jul 2021 05:04:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Abhishek Rawat (Code Review)" <ge...@cloudera.org>.
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 <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Jun 2021 19:14:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17610 )
Change subject: IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
......................................................................
IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
The patch fixes a bug in the function of orderConjunctsByCost, which
could remove the wrong object in the list when the first conjunct is
not the best and there is a same conjunct with different letter cases.
It could end up to have duplicate objects after reordering the list
because the conjunct, which has been added to the return list, is
still in the remaining list, and lead to a wrong plan later where
each side of the JOIN references columns from the other side due to
a double flip on a same conjunct (There are two conjuncts in the list,
and they are flipped as required by the analyzer, but unfortunately,
the two conjuncts are the same object).
The root cause of the issue is that some parts of the analyzer are
case-sensitive, but some parts are not. For example, the remove() of
the List considers the conjuncts with different letter cases are the
same because they refer the same columns, while the compareTo()
of the String considers the letter cases. This discrepancy creates
some unexpected bugs.
The fix uses the index instead of the Object to remove in the
remaining list to solve the bug. However, there may still be
somewhere else in our code that has similar issues regarding to
different letter cases, it could be better that we have a consistent
policy in SQL analyzing to avoid such bugs.
Regression testcases has been added to queries/tpch-outer-joins and
PlannerTest/join-order.
Tests:
Ran the Core FE_TEST and EE_TEST.
Passed the regression test in tpch-outer-joins and
PlannerTest/join-order.
Change-Id: I2ba031d7a6eda21a77b0e53bc41772ee9e00a528
Reviewed-on: http://gerrit.cloudera.org:8080/17610
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/tpch/queries/tpch-outer-joins.test
3 files changed, 80 insertions(+), 3 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I2ba031d7a6eda21a77b0e53bc41772ee9e00a528
Gerrit-Change-Number: 17610
Gerrit-PatchSet: 7
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen 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 4: Code-Review+2
Looks great!
--
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: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Jul 2021 16:02:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6: Verified+1
--
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: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 19:18:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/17610 )
Change subject: IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
......................................................................
IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
The patch fixes a bug in the function of orderConjunctsByCost, which
could remove the wrong object in the list when the first conjunct is
not the best and there is a same conjunct with different letter cases.
It could end up to have duplicate objects after reordering the list
because the conjunct, which has been added to the return list, is
still in the remaining list, and lead to a wrong plan later where
each side of the JOIN references columns from the other side due to
a double flip on a same conjunct (There are two conjuncts in the list,
and they are flipped as required by the analyzer, but unfortunately,
the two conjuncts are the same object).
The root cause of the issue is that some parts of the analyzer are
case-sensitive, but some parts are not. For example, the remove() of
the List considers the conjuncts with different letter cases are the
same because they refer the same columns, while the compareTo()
of the String considers the letter cases. This discrepancy creates
some unexpected bugs.
The fix uses the index instead of the Object to remove in the
remaining list to solve the bug. However, there may still be
somewhere else in our code that has similar issues regarding to
different letter cases, it could be better that we have a consistent
policy in SQL analyzing to avoid such bugs.
Regression testcases has been added to queries/tpch-outer-joins and
PlannerTest/join-order.
Tests:
Ran the Core FE_TEST and EE_TEST.
Passed the regression test in tpch-outer-joins and
PlannerTest/join-order.
Change-Id: I2ba031d7a6eda21a77b0e53bc41772ee9e00a528
---
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/tpch/queries/tpch-outer-joins.test
3 files changed, 80 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/17610/4
--
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: newpatchset
Gerrit-Change-Id: I2ba031d7a6eda21a77b0e53bc41772ee9e00a528
Gerrit-Change-Number: 17610
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9105/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Jul 2021 13:07:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang 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:
(1 comment)
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@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 predicates are not de-duplicated?
--
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 <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 29 Jun 2021 07:45:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7330/ DRY_RUN=false
--
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: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 21 Jul 2021 13:07:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8958/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Jun 2021 17:28:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen 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:
(1 comment)
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: toLowerCase()
If SQL identifiers (such as column names) are involved during comparison, I think we need to distinguish regular identifiers, such as INT_col or int_col which are case insensitive, from delimitated identifiers, such as "INT_col" or "int_col" which are case sensitive. In the example, INT_COL is equal to int_col. But "INT_col" is not equal to "int_col".
--
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 <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Mon, 05 Jul 2021 17:04:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5: Code-Review+2
--
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: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Jul 2021 16:03:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen 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:
(2 comments)
Looks good!
http://gerrit.cloudera.org:8080/#/c/17610/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/17610/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1011
PS2, Line 1011: int bestConjunctIdx = 0;
May add a comment here to indicate that 'bestConjunctIdx' uniquely identifies the best conjunct in 'remaining' list even in the case of ties, and possibly the JIRA IMPALA-9338.
http://gerrit.cloudera.org:8080/#/c/17610/2/testdata/workloads/tpch/queries/tpch-outer-joins.test
File testdata/workloads/tpch/queries/tpch-outer-joins.test:
http://gerrit.cloudera.org:8080/#/c/17610/2/testdata/workloads/tpch/queries/tpch-outer-joins.test@62
PS2, Line 62:
Can we add several new test cases which exercise the combination of regular and delimited column identifiers?
For example, a small test table T with 4 columns: col1, col2, `Col1`, and `col2` and some data. And a couple test queries
1.
select * from t a, t b where
a.col1 = b.col2 and a.`Col1` = b.`Col2`
2.
select * from t a, t b where
a.col1 = b.`Col2` and a.`Col1` = b.Col2
--
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 <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 09 Jul 2021 14:27:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
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 <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 09 Jul 2021 04:43:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/17610 )
Change subject: IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
......................................................................
IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
The patch fixes a bug in the function of orderConjunctsByCost, which
could remove the wrong object in the list when the first conjunct is
not the best and there is a same conjunct with different letter cases.
It could end up to have duplicate objects after reordering the list
because the conjunct, which has been added to the return list, is
still in the remaining list, and lead to a wrong plan later where
each side of the JOIN references columns from the other side due to
a double flip on a same conjunct (There are two conjuncts in the list,
and they are flipped as required by the analyzer, but unfortunately,
the two conjuncts are the same object).
The root cause of the issue is that some parts of the analyzer are
case-sensitive, but some parts are not. For example, the remove() of
the List considers the conjuncts with different letter cases are the
same because they refer the same columns, while the compareTo()
of the String considers the letter cases. This discrepancy creates
some unexpected bugs.
The fix uses the index instead of the Object to remove in the
remaining list to solve the bug. However, there may still be
somewhere else in our code that has similar issues regarding to
different letter cases, it could be better that we have a consistent
policy in SQL analyzing to avoid such bugs.
Regression testcases has been added to queries/tpch-outer-joins and
PlannerTest/join-order.
Tests:
Ran the Core FE_TEST and EE_TEST.
Passed the regression test in tpch-outer-joins and
PlannerTest/join-order.
Change-Id: I2ba031d7a6eda21a77b0e53bc41772ee9e00a528
---
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/tpch/queries/tpch-outer-joins.test
3 files changed, 49 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/17610/2
--
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: newpatchset
Gerrit-Change-Id: I2ba031d7a6eda21a77b0e53bc41772ee9e00a528
Gerrit-Change-Number: 17610
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-9338 Fix impala crashing in impala::RowDescriptor::TupleIsNullable(int)
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
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:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17610/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/17610/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1011
PS2, Line 1011: int bestConjunctIdx = 0;
> May add a comment here to indicate that 'bestConjunctIdx' uniquely identifi
Done
http://gerrit.cloudera.org:8080/#/c/17610/2/testdata/workloads/tpch/queries/tpch-outer-joins.test
File testdata/workloads/tpch/queries/tpch-outer-joins.test:
http://gerrit.cloudera.org:8080/#/c/17610/2/testdata/workloads/tpch/queries/tpch-outer-joins.test@62
PS2, Line 62:
> Can we add several new test cases which exercise the combination of regular
Since we don't distinct delimited columns with different letter cases now(`Col1` and `col1` are treated as the same), added a special testcase using reserved word.
--
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 <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Jul 2021 12:45:43 +0000
Gerrit-HasComments: Yes