Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20184 )
Change subject: IMPALA-10173: (Addendum) Fix substitution for unsafe expressions, column-level compatibility check ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/20184/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20184/3//COMMIT_MSG@10 PS3, Line 10: CastExrp Nit: type, should be CastExpr. http://gerrit.cloudera.org:8080/#/c/20184/3//COMMIT_MSG@12 PS3, Line 12: Before this change Optional: I'd start the commit message with the problem and then describe how it's fixed. http://gerrit.cloudera.org:8080/#/c/20184/3//COMMIT_MSG@16 PS3, Line 16: 'select "1", "1" union : select 1, "1"' Nit: I'd put the example on a separate line. http://gerrit.cloudera.org:8080/#/c/20184/3//COMMIT_MSG@21 PS3, Line 21: it will allow cases Could you make it clearer that this didn't work before the patch? I'd start with the problem and then explain how it's solved. http://gerrit.cloudera.org:8080/#/c/20184/3//COMMIT_MSG@24 PS3, Line 24: 'select "1", 1 union select 1, int_col from : unsafe_insert' Nit: I'd put the example on a separate line. 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@56 PS2, Line 56: private TypeCompatibility compatibility_ = TypeCompatibility.DEFAULT; > Unfortunately, it can't, it's initialized with a default value, and assigne In this case I think it would be more correct if there wasn't a default value here but it would be initialised in each constructor. This is a value that should never change. 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 > The 1-argument base method has a throws clause, that would generate 14 more Ok. http://gerrit.cloudera.org:8080/#/c/20184/3/testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test File testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test: http://gerrit.cloudera.org:8080/#/c/20184/3/testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test@213 PS3, Line 213: ==== Could add a comment that this is a case where one column is needs unsafe compatibility and the other needs default compatibility. -- 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: 3 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-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Comment-Date: Wed, 26 Jul 2023 14:15:22 +0000 Gerrit-HasComments: Yes
