Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/22407 )
Change subject: IMPALA-12588: Don't UPDATE rows that already have the desired value ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/22407/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22407/2//COMMIT_MSG@12 PS2, Line 12: them Nit: line too long. http://gerrit.cloudera.org:8080/#/c/22407/2//COMMIT_MSG@30 PS2, Line 30: where I think we should capitalise it: "WHERE". Also the other instances where we refer to WHERE predicates or the statement. http://gerrit.cloudera.org:8080/#/c/22407/2//COMMIT_MSG@49 PS2, Line 49: For some cases it can be trickier (e.g. UPDATE FROM), those cases Could you include what happens with those cases after this patch? Do they remain as they were? http://gerrit.cloudera.org:8080/#/c/22407/2/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java File fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java: http://gerrit.cloudera.org:8080/#/c/22407/2/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java@54 PS2, Line 54: needsWherePredicateRewrite_ To me, this name suggests that there are cases where the WHERE predicate needs to be rewritten while in others it doesn't need to be. I'd consider 'wherePredicateAlreadyRewritten' or 'wherePredicateRewriteDone'. http://gerrit.cloudera.org:8080/#/c/22407/2/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java@141 PS2, Line 141: where Capitalise WHERE. http://gerrit.cloudera.org:8080/#/c/22407/2/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java@146 PS2, Line 146: 10 This could be extracted into a constant. http://gerrit.cloudera.org:8080/#/c/22407/2/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java@149 PS2, Line 149: predicates You could add more details to what the predicates are, especially as this tree is a bit different from the one in the commit message. There, 'a', 'b' and 'c' are the new values we're setting, and here they are the predicates 'col IS DISTINCT FROM value_a' etc. http://gerrit.cloudera.org:8080/#/c/22407/2/testdata/workloads/functional-query/queries/QueryTest/iceberg-update-basic.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-update-basic.test: http://gerrit.cloudera.org:8080/#/c/22407/2/testdata/workloads/functional-query/queries/QueryTest/iceberg-update-basic.test@332 PS2, Line 332: ref_view Can we try add tests with the two other kinds of views: nested query and WITH statement views? In some places they are handled differently. -- To view, visit http://gerrit.cloudera.org:8080/22407 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I926c80e8110de5a4615a3624a81a330f54317c8b Gerrit-Change-Number: 22407 Gerrit-PatchSet: 2 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 12 Feb 2025 12:26:20 +0000 Gerrit-HasComments: Yes
