-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix failure when converting json stats to parquet in Delta Lake #15517
Conversation
@@ -110,7 +110,13 @@ public static Object jsonValueToTrinoValue(Type type, @Nullable Object jsonValue | |||
return (long) (int) jsonValue; | |||
} | |||
if (type == BIGINT) { | |||
return (long) (int) jsonValue; | |||
if (jsonValue instanceof Long) { | |||
return (long) jsonValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (IntelliJ eye candy) : can we add here //noinspection RedundantCast
same as here
trino/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTypes.java
Lines 146 to 149 in 01270ca
if (icebergType instanceof Types.LongType) { | |
//noinspection RedundantCast | |
return (long) value; | |
} |
} | ||
if (jsonValue instanceof Integer) { | ||
return (long) (int) jsonValue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried out locally (snippet is used in io.trino.plugin.deltalake.transactionlog.statistics.DeltaLakeJsonFileStatistics
)
parseJson(OBJECT_MAPPER, "{\"numRecords\":1,\"minValues\":{\"col\":0},\"maxValues\":{\"col\":9223372036854775807324},\"nullCount\":{\"col\":0}}", DeltaLakeJsonFileStatistics.class)
and the parsed value has BigInteger
.
However, in this case, I agree that an exception should be thrown because the value provided is greater than bigint
max value in Trino https://trino.io/docs/current/language/types.html#bigint
{"integer", "1", "2147483647", "0.0", "1", "2147483647"}, | ||
{"tinyint", "2", "127", "0.0", "2", "127"}, | ||
{"smallint", "3", "32767", "0.0", "3", "32767"}, | ||
{"bigint", "1000", "9223372036854775807", "0.0", "1000", "9223372036854775807"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add
{"bigint", "1001", "2147483647", "0.0", "1001", "2147483647"},
in order to test also the branch used to test also the branch if (jsonValue instanceof Integer) {
in io.trino.plugin.deltalake.transactionlog.DeltaLakeParquetStatisticsUtils#jsonValueToTrinoValue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both branches are already covered in the existing case. 1000 goes to Integer and 9223372036854775807 goes to Long.
92e338e
to
d007838
Compare
4d6d895
to
a4ed68d
Compare
Just rebased on upstream without changes. |
a4ed68d
to
f6b4863
Compare
Description
Fixes #15496
Release notes
(x) Release notes are required, with the following suggested text: