fix: Ensure ListingTable partitions are pruned when filters are not used #17958
Conversation
github-actions
bot
added
core
| .into_iter() | ||
| .filter(|p| { | ||
| let cols = partition_cols.iter().map(|x| x.0.as_str()); | ||
| !parse_partitions_for_path(table_path, &p.path, cols) |
There was a problem hiding this comment.
Do we have to worry about filters such as <partition column> IS NULL ?
There was a problem hiding this comment.
This is a good question, and has made me open a little can of worms.
For the filtering case, it looks like we do not infer the filter for <partition column> IS NULL, and the column is defined as not nullable when the listing table is built. We should probably fix this to support null columns, but we'll need to introduce some configuration for users to specify the null fallback value outside of the default __HIVE_DEFAULT_PARTITION__.
For the non-filtering case, this PR will indeed match null partition column values in the current implementation - but because we don't have any special treatment of them, it would return as the literal text "__HIVE_DEFAULT_PARTITION__" for example. If your partition column is then set as an Int32 for example, the query will fail.
I think implementing proper support for the nulls will need more work outside of this PR. Because we already define the column as non-nullable, what do you think about manually excluding __HIVE_DEFAULT_PARTITION__ values from the parse_partitions_for_path to prevent query errors like the one I describe above, until proper support for nulls is added? I can raise an issue and start working on it as well.
This won't help people with custom null fallback values, but would help for all default cases.
There was a problem hiding this comment.
It looks like this is an existing issue before this PR as well, a non-filter scan (e.g. SELECT <partition column> FROM blah) will also match all files including the null fallback partition:
DataSourceExec: file_groups={1 group: [[peasee-hive-test/pid=1/data.parquet, peasee-hive-test/pid=2/data.parquet, peasee-hive-test/pid=__HIVE_DEFAULT_PARTITION__/data.parquet]]
There was a problem hiding this comment.
It looks like this is an existing issue before this PR as well, a non-filter scan (e.g. SELECT FROM blah) will also match all files including the null fallback partition:
It seems like reasonable behavior to me that SELECT <partition column> FROM blah would also return data without a value for the partition column (as a value of NULL).
But that being said I am not sure even what defines the "expected" behavior in this case (e.g. what does Hive do in this case 🤔 )
There was a problem hiding this comment.
Yes, I should've been clearer that the SELECT <partition column> FROM blah works but it returns the value as __HIVE_DEFAULT_PARTITION__ for example - we don't convert them to NULL yet, which I think we probably ought to.
Hive itself seems to not convert the value at all, suggested by this open issue. I struggled to find direct references to HIVE_DEFAULT_PARTITION in the Hive docs, I only came across this ingestion guide which explains how the Hive writer rewrites NULL into the HIVE_DEFAULT_PARTITION string.
I guess it is up to the implementer to decide what to do with that, because a text partition on a partition column you expect to be an integer could be annoying 😕 It looks like Impala decided to treat them as NULL values: IMPALA-252.
I found the Impala ticket via this Spark issue and associated GitHub PR apache/spark#17277 which also had an interesting discussion about this.
The side affect both of these tickets mention is that Hive does not differentiate between an empty string or a null value, so there is no way to tell which one a HIVE_DEFAULT_PARTITION is.
3 participants
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?