Fix varchar to numeric coercion for hive tables #20766
Conversation
Praveen2112
force-pushed
the
praveen/varchar_to_numeric_coercer_2
branch
3 times, most recently
from
928b247 to
258b683
Compare
Praveen2112
requested review from
kokosing,
krvikash,
findinpath and
vlad-lyutenko
...o-hive/src/test/java/io/trino/plugin/hive/coercions/TestVarcharToIntegralNumericCoercer.java
Show resolved
Hide resolved
| toType.writeLong(blockBuilder, Long.parseLong(valueToBeCoerced)); | ||
| } | ||
| else { | ||
| throw new TrinoException(NOT_SUPPORTED, format("Could not create Coercer from varchar to %s", toType)); |
There was a problem hiding this comment.
nit: message is a bit misleading because it may hint the user that an internal error happened, while actually the coercion is not available.
There was a problem hiding this comment.
actually we can fail early , in the constructor if toType is not any of tinyint, smallint, integer, bigint
| assertVarcharToIntegralCoercion("9223372036854775807", BIGINT, 9223372036854775807L); | ||
| assertVarcharToIntegralCoercion("-9223372036854775808", BIGINT, -9223372036854775808L); | ||
| assertVarcharToIntegralCoercion("9223372036854775808", BIGINT, null); // Greater than Long.MAX_VALUE | ||
| assertVarcharToIntegralCoercion("-9223372036854775809", BIGINT, null); // Lesser than Short.MIN_VALUE |
There was a problem hiding this comment.
// Lesser than Short.MIN_VALUE
please adapt
| @@ -152,6 +152,14 @@ protected void doTestHiveCoercion(HiveTableDefinition tableDefinition) | |||
| "long_decimal_to_varchar", | |||
| "short_decimal_to_bounded_varchar", | |||
| "long_decimal_to_bounded_varchar", | |||
| "varchar_to_tinyint", | |||
| "string_to_tinyint", | |||
There was a problem hiding this comment.
hmmm... we've unfortunately missed covering these use-cases previously :(
good catch Praveen.
Praveen2112
force-pushed
the
praveen/varchar_to_numeric_coercer_2
branch
from
258b683 to
d3eaee1
Compare
|
@findinpath Thanks for your review. AC |
|
Does this need a release note? I think it's fixing the PRs that were included in this release already and users never had it "unfixed," right? |
|
There are two parts here - |
Description
Fixes the varchar to numeric coercion for non-orc tables and also add support for those coercion in case of partitioned table.
For non ORC file format, for coercing from string/varchar
Integerrepresentation of the string and then cast them tobyteorshortbased on the data type. If the number representation is beyond the range of integer, we treat them as nullInteger/Longrepresentation of the string, if it crosses the threshold we treat them as null.For ORC file format, for coercing from string/varchar
If the number representation crosses the limit, we treat them as null.
For any non-varchar representation, we treat them as null.
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: