You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Peter Rozsa (Code Review)" <ge...@cloudera.org> on 2023/05/13 12:35:17 UTC
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
Peter Rozsa has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19881
Change subject: IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statement accepts unsafe implicitly casted expression only when
the source expression is constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting between string types as well:
- String -> Char(n)
- String -> Varchar(n)
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,083 insertions(+), 347 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/19881/1
--
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: newchange
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 1
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
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 8: Code-Review+1
(2 comments)
Thanks, looks good, just a nit.
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@25
PS7, Line 25: truncation
> As the main goal of this change is to add Hive-like implicit cast capabilit
Ok, we should follow Hive here.
http://gerrit.cloudera.org:8080/#/c/19881/8/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/8/fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java@24
PS8, Line 24: STRING
Nit: I think this should be 'string' because it doesn't refer to the concrete STRING type but generally to string-like types.
--
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: 8
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Tue, 27 Jun 2023 08:51:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13040/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 3
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 May 2023 07:00:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statement accepts unsafe implicitly casted expression only when
the source expression is constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting between string types as well:
- String -> Char(n)
- String -> Varchar(n)
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,087 insertions(+), 375 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/19881/3
--
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: newpatchset
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 8:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/13399/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 8
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Tue, 27 Jun 2023 08:23:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13194/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 4
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Jun 2023 08:10:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statements and set operations accept unsafe implicitly casted
expressions only when the source expressions are constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting in set operations and insert statements between string
types:
- String -> Char(n)
- String -> Varchar(n)
The unsafe implicit casting requires that the source expression must be
constant in this case as well.
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,211 insertions(+), 381 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/19881/7
--
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: newpatchset
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 7
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statements and set operations accept unsafe implicitly casted
expressions only when the source expressions are constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting in set operations and insert statements between string
types:
- String -> Char(n)
- String -> Varchar(n)
The unsafe implicit casting requires that the source expression must be
constant in this case as well.
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,218 insertions(+), 381 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/19881/9
--
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: newpatchset
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 9
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statements and set operations accept unsafe implicitly casted
expressions only when the source expressions are constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting in set operations and insert statements between string
types:
- String -> Char(n)
- String -> Varchar(n)
The unsafe implicit casting requires that the source expression must be
constant in this case as well.
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Reviewed-on: http://gerrit.cloudera.org:8080/19881
Reviewed-by: Daniel Becker <da...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,217 insertions(+), 382 deletions(-)
Approvals:
Daniel Becker: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 12
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
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 11: Code-Review+2
--
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: 11
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 29 Jun 2023 14:23:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13434/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 10
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 29 Jun 2023 13:26:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Daniel Becker, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19881
to look at the new patch set (#11).
Change subject: IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statements and set operations accept unsafe implicitly casted
expressions only when the source expressions are constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting in set operations and insert statements between string
types:
- String -> Char(n)
- String -> Varchar(n)
The unsafe implicit casting requires that the source expression must be
constant in this case as well.
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,217 insertions(+), 382 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/19881/11
--
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: newpatchset
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 11
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Daniel Becker, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19881
to look at the new patch set (#10).
Change subject: IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statements and set operations accept unsafe implicitly casted
expressions only when the source expressions are constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting in set operations and insert statements between string
types:
- String -> Char(n)
- String -> Varchar(n)
The unsafe implicit casting requires that the source expression must be
constant in this case as well.
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,217 insertions(+), 382 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/19881/10
--
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: newpatchset
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 10
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
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 <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Jun 2023 11:21:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statement accepts unsafe implicitly casted expressions only when
the source expression is constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting in set operations and insert statements between string
types:
- String -> Char(n)
- String -> Varchar(n)
The unsafe implicit casting requires that the source expression must be
constant in this case as well.
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,204 insertions(+), 381 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/19881/4
--
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: newpatchset
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 4
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statements and set operations accept unsafe implicitly casted
expressions only when the source expressions are constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting in set operations and insert statements between string
types:
- String -> Char(n)
- String -> Varchar(n)
The unsafe implicit casting requires that the source expression must be
constant in this case as well.
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,213 insertions(+), 381 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/19881/8
--
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: newpatchset
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 8
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 11: Verified+1
--
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: 11
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 29 Jun 2023 18:45:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13195/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 5
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Jun 2023 08:17:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate 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 10: Code-Review+2
Thanks Peter, LGTM!
--
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: 10
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 29 Jun 2023 13:29:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13436/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 11
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 29 Jun 2023 14:09:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
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 5:
(32 comments)
Thanks for the replies.
http://gerrit.cloudera.org:8080/#/c/19881/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19881/5//COMMIT_MSG@15
PS5, Line 15: set operations
From this it seems that for set operations the casted expressions don't need to be constant, but at the end of the commit message it says that (for string->[var]char conversions) in inserts the expressions need to be const. Are there other conversions for which the exprs don't need to be const or does the constness requirement also apply to all set operations? It should be made clear in the commit message.
http://gerrit.cloudera.org:8080/#/c/19881/5/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/19881/5/common/thrift/ImpalaService.thrift@794
PS5, Line 794: // Allowing implicit casts with loss of precision.
Nit: empty line before this.
http://gerrit.cloudera.org:8080/#/c/19881/5/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/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3281
PS5, Line 3281: if (compatibility.isUnsafe()) {
In this overload we check constness in case of UNSAFE compatibility, but in the overload in L3264 we don't. Is there a reason for that?
I would think that we should always check constness with UNSAFE compatibility (unless there are cases where non-const expressions are allowed with UNSAFE, but so far I've thought there aren't).
But then the problem arises what happens if there are non-const expressions but there is a common type also with regular compatibility. Maybe we should fall back to the regular compatibility in these cases, or the opposite, always try with regular first and only try permissive if it fails. If I understand it correctly L3389 is an example of that, but other parts in the code don't do that. Maybe we should push it down to Type.getAssignmentCompatibleType() and/or ScalarType.getAssignmentCompatibleType() so that no caller calls them without constness checks, for example Expr.castTo().
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3284
PS5, Line 3284: Optional<Expr> rightNonConstExpr = expr.getFirstNonConstSourceExpr();
Optional: we only need to get the right first non-const expr if the left one doesn't exist, so it could also be
Optional<Expr> nonConstExpr = lastCompatibleExpr.getFirstNonConstSourceExpr();
if (!nonConstExpr.isPresent()) {
Optional<Expr> nonConstExpr = expr.getFirstNonConstSourceExpr();
}
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3389
PS5, Line 3389: compatibleType = getCompatibleType(
Do we know of a case where getCompatibleType() gives (non-error) results for both regular and permissive compatibility and the results are different? Is it the reason we try it with the regular compatibility first?
Or is it because if we try it with UNSAFE first and it contains non-const exprs it throws?
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3407
PS5, Line 3407: getCompatibleType
With this overload it's not checked whether the expression contains non-const subexpressions, shouldn't it be?
Also, if there are non-const subexpressions, shouldn't we try to check the compatible type with regular compatibility here? The overall compatible type may have been found with regular compatibility on L3389.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3451
PS5, Line 3451: compatibility
The comment should explain what the function does with 'compatibility'.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3458
PS5, Line 3458: public TypeCompatibility getRegularCompatibilityLevel() {
Nit: empty line before function.
http://gerrit.cloudera.org:8080/#/c/19881/5/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/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@659
PS5, Line 659: TypeCompatibility.STRICT_DECIMAL
Is it only possible to call this function with TypeCompatibility.STRICT_DECIMAL or TypeCompatibility.DEFAULT? Can't it be called for example with TypeCompatibility.ALL_STRICT? I think instead of the enum values we should refer to TypeCompatibility.isStrictDecimal() here.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1114
PS5, Line 1114: getPermissiveCompatibilityLevel
Can we get here if we're not in an INSERT or SET OPERATION? Permissive compatibility should only apply to those cases.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1559
PS5, Line 1559: compatibility
See L1114 about permissive compatibility. Also, constness should be checked (see the comment at Analyzer.java L3281) - or do we allow non-const expressions here?
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1943
PS5, Line 1943: 1. checking whether
We only do step 1. if there is no underlying slot ref.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1943
PS5, Line 1943: itself constant
Nit: ... itself is constant ...
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1951
PS5, Line 1951: !
It would be clearer if we removed the negation ('!') and reversed the order of the result operands.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1951
PS5, Line 1951: isConstant
The comment of the isConstant() function says that it may be unreliable before the expression is analyzed. Do we know if the expression is analyzed here? We could insert a Precondition check for that and run some tests.
Unfortunately the comment doesn't tell us more about the uncertainty, for example if only false negatives can occur or also false positives.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1952
PS5, Line 1952: return nonConstExpr;
Could return the expression directly, without assigning it to 'nonConstExpr'.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1953
PS5, Line 1953: else
No need to put it in an ELSE block as the IF block always returns. 'nonConstExpr' could be declared after the IF.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1954
PS5, Line 1954: isConstant
It slotRef.isConstant() is true, can't we early-return Optional.empty() here?
In this case we could also eliminate the 'nonConstExpr' variable.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1954
PS5, Line 1954: !
Nit: remove negation and reverse result operands.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@1967
PS5, Line 1967: findFirst
If the comment on L1954 about early-returning is correct, we only reach here if the slot ref is not constant. In this case the value returned by findFirst() should not be empty, we should add a Precondition check for it.
(Or if, for some reason, it's possible for a slot ref to not be const if all source expressions are const, in this case we should return the slot ref as the first non-const expr. Maybe we should check how 'isConstant_' is set during analysis, that could give more info about what states are possible.)
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/StatementBase.java
File fe/src/main/java/org/apache/impala/analysis/StatementBase.java:
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@216
PS5, Line 216: compatibility
We called it 'regularCompatibility()' in Analyzer.java, I think that's a better name.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
File fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java:
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java@24
PS5, Line 24: from
It can't be cast from STRING because of L36, right?
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
File fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java:
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java@25
PS5, Line 25: apply
I think it would be better on a new line.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
File fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java:
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java@38
PS3, Line 38: matrix[BOOLEAN.ordinal()][TINYINT.ordinal()] = PrimitiveType.TINYINT;
> matrix[TINYINT.ordinal()][BOOLEAN.ordinal()] cannot happen, the size of TIN
I think we should either fill the matrix symmetrically or leave a comment that the matrix is conceptually symmetrical and types should/will be looked up in a defined order. We could also refer to the comment of Type.compatibilityMatrix.
Filling the matrix symmetrically feels better because in this case default and permissive matrices can be handled uniformly, though of course it has some cost in terms of code, (performance) and effort because if the matrix becomes symmetrical the ordered lookup we use now becomes unnecessary and should be removed.
On the other hand, the default and strict matrices are used to get a common type, and the permissive matrix is used to check whether a type can be converted to another (if I understand it correctly), so they are used differently anyways. But a comment should describe the situation.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
File fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java:
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java@80
PS5, Line 80: the
Nit: in the?
http://gerrit.cloudera.org:8080/#/c/19881/5/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/5/fe/src/main/java/org/apache/impala/catalog/Type.java@675
PS5, Line 675: compatibilityMatrix
It could be added to the comment that types are to be looked up in a specific order ("smaller" first) (if the matrix is not modified to be symmetrical).
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/catalog/Type.java@684
PS5, Line 684: * If unsafe mode is enabled, 'unsafeCompatibilityMatrix' is used for lookup. This
It could be added that it is not actually used to find a compatible type but check whether conversion in the given direction is allowed.
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/catalog/Type.java@687
PS5, Line 687: ,
Nit: dot instead of comma.
http://gerrit.cloudera.org:8080/#/c/19881/3/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/3/fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java@21
PS3, Line 21:
> It gets truncated to the limit of the target type, maybe the "Possible loss
This truncating behaviour should be mentioned here.
http://gerrit.cloudera.org:8080/#/c/19881/5/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/5/fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java@56
PS5, Line 56: // Unreachable state
Can't the Java compiler see that the switch is exhaustive so no need for a default branch?
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
File fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java:
http://gerrit.cloudera.org:8080/#/c/19881/5/fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java@81
PS5, Line 81: // CHAR, VARCHAR to STRING
What about STRING to CHAR, VARCHAR? In TypeCompatibility.java it is written that STRINGs can be converted to CHARs and VARCHARs.
http://gerrit.cloudera.org:8080/#/c/19881/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/19881/3/testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test@39
PS3, Line 39:
> Yes, the common type for the values clause will be DECIMAL(6,1). The type c
Ok, can you open an issue for it so we know about it?
--
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: 5
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Jun 2023 13:51:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9463/ DRY_RUN=false
--
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: 11
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 29 Jun 2023 14:23:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19881/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/19881/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@3383
PS1, Line 3383: "insert into functional.alltypesnopart(%s_col) select string_col from functional.alltypes",
line too long (103 > 90)
--
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: 1
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 13 May 2023 12:36:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13030/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 13 May 2023 13:06:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
......................................................................
Patch Set 3:
(41 comments)
Thanks Péter!
http://gerrit.cloudera.org:8080/#/c/19881/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19881/3//COMMIT_MSG@7
PS3, Line 7: from
We could change this to "between".
http://gerrit.cloudera.org:8080/#/c/19881/3//COMMIT_MSG@16
PS3, Line 16: expression
Nit: expressions (or alternatively "accepts an unsafe ... expression").
http://gerrit.cloudera.org:8080/#/c/19881/3//COMMIT_MSG@25
PS3, Line 25: The patch also covers IMPALA-3217, and adds two more rules to handle
Is it only in the case of INSERT statements or also SET OPERATIONs? If only INSERT, it should be mentioned.
I presume it's only possible with constants.
http://gerrit.cloudera.org:8080/#/c/19881/3/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/19881/3/common/thrift/ImpalaService.thrift@794
PS3, Line 794: // Allowing implicit casts with loss of precision
We could explain it in more detail here, e.g. what kinds of conversions are allowed. It's written in the commit message but that's not readily available to someone reading the code later.
http://gerrit.cloudera.org:8080/#/c/19881/3/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/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3263
PS3, Line 3263: public Type getCompatibleType(Type leftType, Expr leftExpr, Type rightType,
Could this be static?
Is it needed in addition to the below 4-argument version because 'rightExpr' may not have the 'rightType' type?
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3271
PS3, Line 3271: public Type getCompatibleType(Type lastCompatibleType, Expr lastCompatibleExpr,
Could this be static?
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3285
PS3, Line 3285: private void checkIncompatibleType(Type leftType, Expr leftExpr, Type rightType,
Could this be static?
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3411
PS3, Line 3411: public TypeCompatibility getCompatibility() {
I'm not sure about the name of this method and getUnsafeCompatibility(). getUnsafeCompatibility() sounds like it returns the constant TypeCompatibility.UNSAFE.
If I understand it correctly, in some situations a "regular" compatibility level is used and in other situations a more permissive one. Maybe renaming these functions to "getRegularCompatibilityLevel()" and "getPermissiveCompatibilityLevel()" is better, but only slightly... If we can't come up with better names, at least we should leave a doc comment for these functions that explains the situation.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java:
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java@296
PS3, Line 296: analyzer.getCompatibility()
Here, before the change, the parameter 'strict' was true, but we lose this because analyzer.getCompatibility() is either DEFAULT or STRICT_DECIMAL, never STRICT or ALL_STRICT.
http://gerrit.cloudera.org:8080/#/c/19881/3/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/19881/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@655
PS3, Line 655: * both children will be cast to (11, 3).
Mention 'compatibility'.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@709
PS3, Line 709: STRICT_DECIMAL
'TypeCompatibility.STRICT_DECIMAL' would be clearer.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@709
PS3, Line 709: STRICT_DECIMAL
Is it possible that compatibility is ALL_STRICT? Shouldn't we say "if compatibility.isStrictDecimal() is true then..."?
If it can be ALL_STRICT, L730 changes the meaning as we used to pass 'false' there. If it can't be ALL_STRICT, we could do a precondition check.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1537
PS3, Line 1537: checks the validity of the cast
Should mention that it is checked according to 'compatibility'.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1933
PS3, Line 1933: slotDesc
Shouldn't we check that 'slotDesc' is not NULL? If it should never be NULL, add a precondition check.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1934
PS3, Line 1934: sourceExpressionsAreConstants
It is enough to define it before the next loop, on L1940.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1940
PS3, Line 1940: for (Expr expr : slotDesc.getSourceExprs()) {
You could return false from within the loop if !expr.isConstant() and true after the loop.
If you'd like, you could use a stream and allMatch().
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/RangePartition.java
File fe/src/main/java/org/apache/impala/analysis/RangePartition.java:
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/RangePartition.java@217
PS3, Line 217: analyzer.getCompatibility()
Here, before the change, the parameter 'strict' was true, but we lose this because analyzer.getCompatibility() is either DEFAULT or STRICT_DECIMAL, never STRICT or ALL_STRICT.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/StatementBase.java
File fe/src/main/java/org/apache/impala/analysis/StatementBase.java:
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@202
PS3, Line 202:
Nit: the unsafe option.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
File fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java:
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java@34
PS3, Line 34:
Nit: extra space.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java@36
PS3, Line 36: matrix[STRING.ordinal()][BINARY.ordinal()] = PrimitiveType.INVALID_TYPE;
What about the other direction?
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java@37
PS3, Line 37: matrix[VARCHAR.ordinal()][BINARY.ordinal()] = PrimitiveType.VARCHAR;
Why do we allow conversion to/from VARCHAR? If we do, why don'y we add "i != VARCHAR.ordinal()" in as a condition on L29?
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
File fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java:
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java@30
PS3, Line 30: NULL
It should be NULL_TYPE, NULL is ambiguous, could also mean Java null-ness which we check on L34.
Nit: if INVALID_TYPE is checked first, it would be better if that was written first in the comment.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java@30
PS3, Line 30: ,
Nit: no need for the comma.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java@31
PS3, Line 31: if (t1 == PrimitiveType.INVALID_TYPE || t2 == PrimitiveType.INVALID_TYPE)
If the whole statement doesn't fit on a line, use braces { }.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
File fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java:
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java@20
PS3, Line 20: public interface CompatibilityRule { void apply(PrimitiveType[][] matrix); }
Add a comment describing this interface and the 'apply' method.
This would be a good place to describe how compatibility matrices work, but it could also be done in Type.java - but it should be updated as the meaning has changed a bit, the order of the indices now has a meaning (cast direction) so the matrix is not necessarily symmetrical (if I understand it correctly).
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java@20
PS3, Line 20: apply
I think it would be better on a new line.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
File fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java:
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java@37
PS3, Line 37: public void apply(PrimitiveType[][] matrix) {
Does the order of the indices have a meaning in this matrix?
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java@38
PS3, Line 38: matrix[BOOLEAN.ordinal()][TINYINT.ordinal()] = PrimitiveType.TINYINT;
Don't we leave empty (null) values for the other direction, e.g. matrix[TINYINT.ordinal()][BOOLEAN.ordinal()]?
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java@80
PS3, Line 80: only mantissa
Nit: 'only in the mantissa'
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/ScalarType.java
File fe/src/main/java/org/apache/impala/catalog/ScalarType.java:
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@454
PS3, Line 454: * is INVALID_TYPE.
Mention 'compatibility' in the comment.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@457
PS3, Line 457: compatibility
Note to self: check what becomes of the 'boolean strict' parameter: do we keep the behaviour from before the patch? In some cases it was true, in others it was false before.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@473
PS3, Line 473: if
Could be 'else if'.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@516
PS3, Line 516: * Returns true t1 can be implicitly cast to t2, false otherwise.
Could add "according to 'compatibility'.
http://gerrit.cloudera.org:8080/#/c/19881/3/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/3/fe/src/main/java/org/apache/impala/catalog/Type.java@341
PS3, Line 341: *
Mention 'compatibility'.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/Type.java@358
PS3, Line 358: *
Mention 'compatibility'.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/Type.java@682
PS3, Line 682: protected static PrimitiveType[][] unsafeCompatibilityMatrix;
Write a comment for 'unsafeCompatibilityMatrix', too.
http://gerrit.cloudera.org:8080/#/c/19881/3/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/3/fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java@21
PS3, Line 21: UNSAFE, // allow implicit casts between string types (STRING, VARCHAR, CHAR) and between
The comments would be better on the line(s) before the enum variants.
http://gerrit.cloudera.org:8080/#/c/19881/3/fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java@21
PS3, Line 21: string types
From STRING to [VAR]CHAR types, what do we do if the STRING is too long? Is it only used for constants where we can check it at planning time? If so, it should be mentioned also here.
http://gerrit.cloudera.org:8080/#/c/19881/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/19881/3/testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test@6
PS3, Line 6: INT
Why is it INT? Shouldn't 1.0 be DECIMAL or FLOAT or DOUBLE?
http://gerrit.cloudera.org:8080/#/c/19881/3/testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test@9
PS3, Line 9: INSERT INTO unsafe_insert(string_col) select 1 union select int_col from unsafe_insert;
Here the problem is that 'int_col' is not constant, right? If so, it would be good if the error message said this, blaming it on "1" is misleading as it is no problem that "1" is in the query on L21.
This also applies to the next query and the queries returning error at the bottom: in unsafe mode, if the type is ok but the expression is not constant, this non-constantness should be given as the cause in the error message.
http://gerrit.cloudera.org:8080/#/c/19881/3/testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test@39
PS3, Line 39: 100
Do I understand it correctly that the common type of the values expressions is DECIMAL and that's not compatible with STRING?
Would it be possible to flag the correct expression that is incompatible (1000.0 in this case) or at least give a message that the common type of the expressions is incompatible? Now the message is a bit misleading because "100" wouldn't be a DECIMAL on its own.
--
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: 3
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 May 2023 14:21:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19881/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/19881/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@3380
PS4, Line 3380: "insert into functional.alltypesnopart(%s_col) select string_col from functional.alltypes",
line too long (103 > 90)
http://gerrit.cloudera.org:8080/#/c/19881/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@3396
PS4, Line 3396: "Target table 'functional.alltypesnopart' is incompatible with source expressions.\n"
line too long (93 > 90)
--
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: 4
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Jun 2023 07:50:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statement accepts unsafe implicitly casted expressions only when
the source expression is constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting in set operations and insert statements between string
types:
- String -> Char(n)
- String -> Varchar(n)
The unsafe implicit casting requires that the source expression must be
constant in this case as well.
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,206 insertions(+), 381 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/19881/5
--
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: newpatchset
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
......................................................................
IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
This patch adds an expiremental query option called ALLOW_UNSAFE_CASTS
which allows implicit casting between some numeric types and string
types. A new type of compatibility is introduced for this purpose, and
the compatibility rule handling is refactored also. The new approach
uses an enum to differentiate the compatibility levels, and to make it
easier to pass them through methods. The unsafe compatibility is used
only in two cases: for set operations and for insert statements. The
insert statement accepts unsafe implicitly casted expression only when
the source expression is constant.
The following implicit type casts are enabled in unsafe mode:
- String -> Float, Double
- String -> Tinyint, Smallint, Int, Bigint
- Float, Double -> String
- Tinyint, Smallint, Int, Bigint -> String
The patch also covers IMPALA-3217, and adds two more rules to handle
implicit casting between string types as well:
- String -> Char(n)
- String -> Varchar(n)
Tests:
- tests added to AnalyzeExprsTest.java
- new test class added to test_insert.py
Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
A fe/src/main/java/org/apache/impala/catalog/BinaryCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CheckEmptyCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
A fe/src/main/java/org/apache/impala/catalog/DefaultCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/DiagonalCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/FixedUdaCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
A fe/src/main/java/org/apache/impala/catalog/StrictOverrideCompatibility.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
A fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java
A fe/src/main/java/org/apache/impala/catalog/UnsafeCompatibility.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
A testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test
M tests/query_test/test_insert.py
41 files changed, 1,084 insertions(+), 347 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/19881/2
--
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: newpatchset
Gerrit-Change-Id: Iee5db2301216c2e088b4b3e4f6cb5a1fd10600f7
Gerrit-Change-Number: 19881
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19881 )
Change subject: IMPALA-10173: Allow implicit casts from numerics to STRING when inserting into table
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13029/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 1
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 13 May 2023 12:56:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/13375/ : Initial code review checks failed. See linked job for details on the failure.
--
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 <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Jun 2023 08:28:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate 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 10:
There is a merge conflict, after you resolve it we can start dry_run to validate and merge it.
--
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: 10
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 29 Jun 2023 13:29:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate 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 8:
(3 comments)
Hi Peter, nice change and design!
LGTM, just left some nits on commenting.
http://gerrit.cloudera.org:8080/#/c/19881/8/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/19881/8/common/thrift/ImpalaService.thrift@796
PS8, Line 796: IMPALA-10173
nit: I think there is no need to mention the Jira here in this case, it is available in the history
http://gerrit.cloudera.org:8080/#/c/19881/8/fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java
File fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java:
http://gerrit.cloudera.org:8080/#/c/19881/8/fe/src/main/java/org/apache/impala/catalog/CompatibilityRule.java@24
PS8, Line 24: */
nit: could you add an example?
http://gerrit.cloudera.org:8080/#/c/19881/8/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/8/fe/src/main/java/org/apache/impala/catalog/TypeCompatibility.java@22
PS8, Line 22: TypeCompatibility
I really like this design.
nit: Could you add some basic information on the class in comments. ie.: it is used to decide the whether casts between types are compatible.
--
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: 8
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 29 Jun 2023 10:58:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10173: Allow implicit casts between numeric and string types when inserting into table
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 9:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/13433/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 9
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 29 Jun 2023 12:30:25 +0000
Gerrit-HasComments: No