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 reading NDV using Iceberg metadata on table with NULL partitions #21844
base: master
Are you sure you want to change the base?
Fix reading NDV using Iceberg metadata on table with NULL partitions #21844
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
c16bc98
to
20d7a4d
Compare
|
||
return NullableValue.of(column.getType(), prestoValue); | ||
return partitionColumnValueStrings.get(columnId) | ||
.map(value -> deserializePartitionValue(column.getType(), value, column.getName())) |
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.
Can we add @Nullable
to deserializePartitionValue
method in a separated commit?
partitionColumnValueStrings.get(columnId).orElse(null), | ||
column.getName()); | ||
|
||
return NullableValue.of(column.getType(), prestoValue); |
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.
Did you consider calling the NullableValue
constructor instead?
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.
Good question. I don't have a preference. I don't quite understand why it has both constructor and static methods.
Which way do you think is better?
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.
they have different semantics -- NullableValue.of
doesn't allow null value
(i with the difference was more obvious from the NullableValue class's api)
Description
Use NullableValue.asNull for null values.
Additional context and related issues
Issue can be reproduced when query count distinct on partition with NULL value and optimize_metadata_queries session enabled.
Release notes
(x) Release notes are required, with the following suggested text: