Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20184 )
Change subject: IMPALA-10173: Fix substitution for unsafe expressions, column-level compatibility check ...................................................................... Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/20184/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20184/2//COMMIT_MSG@7 PS2, Line 7: Add (Addendum), as the Jira ticket is not specifically for this change. http://gerrit.cloudera.org:8080/#/c/20184/2//COMMIT_MSG@9 PS2, Line 9: fixes Could you describe what the problem was? A test could be added for a problematic query. http://gerrit.cloudera.org:8080/#/c/20184/2//COMMIT_MSG@10 PS2, Line 10: now We often describe the state before the change as "now" in commit messages. This may not be a good habit, but I think we should be more explicit here, we could say "after this change". http://gerrit.cloudera.org:8080/#/c/20184/2//COMMIT_MSG@12 PS2, Line 12: I'd put the following sentences in a new paragraph, as they address a different problem. http://gerrit.cloudera.org:8080/#/c/20184/2//COMMIT_MSG@13 PS2, Line 13: now See comment on L10. http://gerrit.cloudera.org:8080/#/c/20184/2//COMMIT_MSG@15 PS2, Line 15: , Nit: comma not needed. http://gerrit.cloudera.org:8080/#/c/20184/2//COMMIT_MSG@16 PS2, Line 16: another one contains non-constant expressions with regular casts in one Could also add an example of what didn't use to work but works now. http://gerrit.cloudera.org:8080/#/c/20184/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java File fe/src/main/java/org/apache/impala/analysis/CastExpr.java: http://gerrit.cloudera.org:8080/#/c/20184/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@55 PS2, Line 55: defined Nit: . (full stop) at the end. http://gerrit.cloudera.org:8080/#/c/20184/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@56 PS2, Line 56: private TypeCompatibility compatibility_ = TypeCompatibility.DEFAULT; Could be final. http://gerrit.cloudera.org:8080/#/c/20184/2/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/20184/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@1115 PS2, Line 1115: castExpr Optional: could be 'thisCastExpr'. http://gerrit.cloudera.org:8080/#/c/20184/2/fe/src/main/java/org/apache/impala/analysis/NullLiteral.java File fe/src/main/java/org/apache/impala/analysis/NullLiteral.java: http://gerrit.cloudera.org:8080/#/c/20184/2/fe/src/main/java/org/apache/impala/analysis/NullLiteral.java@48 PS2, Line 48: TypeCompatibility.DEFAULT Optional: you could use the 1-argument overload inherited from Expr. http://gerrit.cloudera.org:8080/#/c/20184/2/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java File fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java: http://gerrit.cloudera.org:8080/#/c/20184/2/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@470 PS2, Line 470: return new CastExpr(targetType, this); Don't we need to use 'compatibility' here? -- To view, visit http://gerrit.cloudera.org:8080/20184 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39d13f177482f74ec39570118adab609444c6929 Gerrit-Change-Number: 20184 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 11 Jul 2023 16:05:07 +0000 Gerrit-HasComments: Yes
