You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Peter Rozsa (Code Review)" <ge...@cloudera.org> on 2025/04/08 14:39:41 UTC
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Peter Rozsa has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22761
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
IMPALA-13932: Add file path and position-based duplicate check for
IcebergMergeNode
IcebergMergeNode's duplicate checking mechanism was based on comparing
pointers of the target table's rows. This mechanism results in
false positives if a new row batch reused the memory of the previous row
batch provided for the merge node. This change adds an additional check
that validates the file position and file path as well.
Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
---
M be/src/exec/iceberg-merge-node.cc
M be/src/exec/iceberg-merge-node.h
M fe/src/main/java/org/apache/impala/analysis/IcebergMergeImpl.java
M testdata/workloads/functional-planner/queries/PlannerTest/iceberg-merge-insert-only.test
4 files changed, 73 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/22761/1
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 1
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22761/3/be/src/exec/iceberg-merge-node.cc
File be/src/exec/iceberg-merge-node.cc:
http://gerrit.cloudera.org:8080/#/c/22761/3/be/src/exec/iceberg-merge-node.cc@284
PS3, Line 284:
> nit: underscore not needed
Done
http://gerrit.cloudera.org:8080/#/c/22761/3/be/src/exec/iceberg-merge-node.cc@299
PS3, Line 299: void IcebergMergeNode::SavePreviousRowPtrAndIdent(TupleRow* actual_row) {
: impala_udf::StringVal file_path_sv =
: delete_meta_evaluators_[0]->GetStringVal(actual_row);
: auto file_pos = delete_meta_evaluators_[1]->GetBigIntVal(actual_row);
> target tuple can be a nullptr in case of WHEN NOT MATCHED BY TARGET, so pro
The evaluator will return a null string in this case, initializing a std::string_view with a nullptr and 0 size is valid, also the file_pos will be 0.
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 4
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Apr 2025 03:52:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18864/ : 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/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 4
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Apr 2025 04:17:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
IcebergMergeNode's duplicate checking mechanism was based on comparing
pointers of the target table's rows. This mechanism results in
false positives if a new row batch reused the memory of the previous row
batch provided for the merge node. This change adds an additional check
that validates the file position and file path as well.
Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
---
M be/src/exec/iceberg-merge-node.cc
M be/src/exec/iceberg-merge-node.h
M fe/src/main/java/org/apache/impala/analysis/IcebergMergeImpl.java
M testdata/workloads/functional-planner/queries/PlannerTest/iceberg-merge-insert-only.test
4 files changed, 66 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/22761/3
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18862/ : 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/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Apr 2025 18:09:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/18860/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Apr 2025 16:14:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 2:
(5 comments)
Thanks, Zoltan!
http://gerrit.cloudera.org:8080/#/c/22761/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22761/1//COMMIT_MSG@8
PS1, Line 8:
> nit: title should be single line
Done
http://gerrit.cloudera.org:8080/#/c/22761/1/be/src/exec/iceberg-merge-node.cc
File be/src/exec/iceberg-merge-node.cc:
http://gerrit.cloudera.org:8080/#/c/22761/1/be/src/exec/iceberg-merge-node.cc@187
PS1, Line 187: // Materialize it as a string
: if (std::holds_alternative<std::string_view>(previous_row_file_path_)) {
: auto* view = std::get_if<std::string_view>(&previous_row_file_path_);
> Would it be the same as?
Unfortunately, the autoconversion will not work this way, I used holds_alternative and get_if to achieve it.
http://gerrit.cloudera.org:8080/#/c/22761/1/be/src/exec/iceberg-merge-node.cc@293
PS1, Line 293: auto file_path_sv = delete_meta_evaluators_[0]->GetStringVal(actual_row);
: auto file_pos = delete_meta_evaluators_[1]->GetBigIntVal(actual_row);
: i
> nit: fits single line
Done
http://gerrit.cloudera.org:8080/#/c/22761/1/be/src/exec/iceberg-merge-node.cc@296
PS1, Line 296:
> nit: underscore is not needed.
Done
http://gerrit.cloudera.org:8080/#/c/22761/1/be/src/exec/iceberg-merge-node.cc@303
PS1, Line 303: }
:
: void IcebergMergeNode::SavePreviousRowPtrAndIdent(TupleRow* actual_row) {
: impala_udf::
> nit: return !different;
Done
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Apr 2025 15:56:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11713/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Apr 2025 03:53:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22761/3/be/src/exec/iceberg-merge-node.cc
File be/src/exec/iceberg-merge-node.cc:
http://gerrit.cloudera.org:8080/#/c/22761/3/be/src/exec/iceberg-merge-node.cc@284
PS3, Line 284: _
nit: underscore not needed
http://gerrit.cloudera.org:8080/#/c/22761/3/be/src/exec/iceberg-merge-node.cc@299
PS3, Line 299: void IcebergMergeNode::SavePreviousRowPtrAndIdent(TupleRow* actual_row) {
: impala_udf::StringVal file_path_sv =
: delete_meta_evaluators_[0]->GetStringVal(actual_row);
: auto file_pos = delete_meta_evaluators_[1]->GetBigIntVal(actual_row);
target tuple can be a nullptr in case of WHEN NOT MATCHED BY TARGET, so probably we'll get segfaults in this funcion
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Apr 2025 19:04:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Apr 2025 09:32:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
IcebergMergeNode's duplicate checking mechanism was based on comparing
pointers of the target table's rows. This mechanism results in
false positives if a new row batch reused the memory of the previous row
batch provided for the merge node. This change adds an additional check
that validates the file position and file path as well.
Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Reviewed-on: http://gerrit.cloudera.org:8080/22761
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Zoltan Borok-Nagy <bo...@cloudera.com>
---
M be/src/exec/iceberg-merge-node.cc
M be/src/exec/iceberg-merge-node.h
M fe/src/main/java/org/apache/impala/analysis/IcebergMergeImpl.java
M testdata/workloads/functional-planner/queries/PlannerTest/iceberg-merge-insert-only.test
4 files changed, 72 insertions(+), 30 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Zoltan Borok-Nagy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 6
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
IcebergMergeNode's duplicate checking mechanism was based on comparing
pointers of the target table's rows. This mechanism results in
false positives if a new row batch reused the memory of the previous row
batch provided for the merge node. This change adds an additional check
that validates the file position and file path as well.
Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
---
M be/src/exec/iceberg-merge-node.cc
M be/src/exec/iceberg-merge-node.h
M fe/src/main/java/org/apache/impala/analysis/IcebergMergeImpl.java
M testdata/workloads/functional-planner/queries/PlannerTest/iceberg-merge-insert-only.test
4 files changed, 69 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/22761/2
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 2: Code-Review+2
LGTM!
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Apr 2025 16:38:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 3: Code-Review+2
LGTM!
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Apr 2025 18:49:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11709/
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Apr 2025 23:49:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 1: Code-Review+2
(5 comments)
Thanks for the quick fix!
http://gerrit.cloudera.org:8080/#/c/22761/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22761/1//COMMIT_MSG@8
PS1, Line 8: IcebergMergeNode
nit: title should be single line
http://gerrit.cloudera.org:8080/#/c/22761/1/be/src/exec/iceberg-merge-node.cc
File be/src/exec/iceberg-merge-node.cc:
http://gerrit.cloudera.org:8080/#/c/22761/1/be/src/exec/iceberg-merge-node.cc@187
PS1, Line 187: auto file_path = delete_meta_evaluators_[0]->GetStringVal(previous_row_);
: previous_row_file_path_ =
: std::string(reinterpret_cast<const char*>(file_path.ptr), file_path.len);
Would it be the same as?
previous_row_file_path_ = std::string(previous_row_file_path_);
http://gerrit.cloudera.org:8080/#/c/22761/1/be/src/exec/iceberg-merge-node.cc@293
PS1, Line 293: if (previous_row_file_pos_ != file_pos.val) {
: return false;
: }
nit: fits single line
http://gerrit.cloudera.org:8080/#/c/22761/1/be/src/exec/iceberg-merge-node.cc@296
PS1, Line 296: _
nit: underscore is not needed.
http://gerrit.cloudera.org:8080/#/c/22761/1/be/src/exec/iceberg-merge-node.cc@303
PS1, Line 303: if (different) {
: return false;
: }
: return true;
nit: return !different;
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 1
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Apr 2025 15:01:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/18859/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 1
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Apr 2025 14:58:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
IcebergMergeNode's duplicate checking mechanism was based on comparing
pointers of the target table's rows. This mechanism results in
false positives if a new row batch reused the memory of the previous row
batch provided for the merge node. This change adds an additional check
that validates the file position and file path as well.
Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
---
M be/src/exec/iceberg-merge-node.cc
M be/src/exec/iceberg-merge-node.h
M fe/src/main/java/org/apache/impala/analysis/IcebergMergeImpl.java
M testdata/workloads/functional-planner/queries/PlannerTest/iceberg-merge-insert-only.test
4 files changed, 72 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/22761/4
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 4
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Apr 2025 09:25:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22761 )
Change subject: IMPALA-13932: Add file path and position-based duplicate check for IcebergMergeNode
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11709/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/22761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71b47414321675958c05438ef3aeeb5df0128033
Gerrit-Change-Number: 22761
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Apr 2025 18:13:21 +0000
Gerrit-HasComments: No