Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/19881/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/19881/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3404 PS7, Line 3404: permissiveCompatibility It could be that 'permissiveCompatibility' is UNSAFE but the expression is non-const and we found the compatible type with 'regularCompatibility' on L3385. This will throw in this case but it shouldn't. Maybe we should store which compatibility we found 'compatibleType' with and use that here and also on L3407, like in the case of StatementBase.checkTypeCompatibility(). http://gerrit.cloudera.org:8080/#/c/19881/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3446 PS7, Line 3446: isStrictDecimal Nit: add parenthesis, i.e. isStrictDecimal(). http://gerrit.cloudera.org:8080/#/c/19881/7/fe/src/main/java/org/apache/impala/catalog/Type.java File fe/src/main/java/org/apache/impala/catalog/Type.java: http://gerrit.cloudera.org:8080/#/c/19881/7/fe/src/main/java/org/apache/impala/catalog/Type.java@675 PS7, Line 675: * 'getAssignmentCompatibleType'. Nit: add parentheses: 'getAssignmentCompatibleType()'. http://gerrit.cloudera.org:8080/#/c/19881/7/fe/src/main/java/org/apache/impala/catalog/Type.java@687 PS7, Line 687: that Nit: unnecessary 'that'. http://gerrit.cloudera.org:8080/#/c/19881/7/fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java File fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java: http://gerrit.cloudera.org:8080/#/c/19881/7/fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java@24 PS7, Line 24: type Nit: types. http://gerrit.cloudera.org:8080/#/c/19881/7/fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java@25 PS7, Line 25: truncation If unsafe casts are only allowed for constants, we could determine the lengths at planning time. Wouldn't it be better to reject casts when the STRING is longer than the [VAR]CHAR length instead of truncation? A truncating implicit cast doesn't sound good to me. -- To view, visit http://gerrit.cloudera.org:8080/19881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7 Gerrit-Change-Number: 19881 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@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: Fri, 23 Jun 2023 11:21:55 +0000 Gerrit-HasComments: Yes
