Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 )
Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate ...................................................................... Patch Set 9: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java File fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java: http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@439 PS9, Line 439: private static boolean isRequireMandatoryRewrite(List<SetOperand> operands) { > Looking again, I think requireMandatoryRewrite_ should not be reset like it Thanks, it makes sense. http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java File fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java: http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java@92 PS9, Line 92: if (!requireMandatoryRewrite_) return; What if we didn't have the 'requireMandatoryRewrite_' field and instead just had the below ExprRewriter in all cases? It would still only rewrite the expressions that need to be rewritten. It would make the code simpler, though maybe a bit less performant (the expr tree would have to be traversed for both rules instead of checking once whether any || or BETWEEN expr occurs in it). What do you think? -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 9 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 12 Sep 2023 09:27:23 +0000 Gerrit-HasComments: Yes
