Conversation
… types (i.e one filter on partition column another not, OR vs. AND)
| for (auto &field : partition.fields) { | ||
| columns.push_back(DuckLakePartitionColumn(field)); | ||
| } | ||
| partition_id = partition.spec_id; |
There was a problem hiding this comment.
I don't think this is correct?
See the comment:
//! The id is assigned after we've processed all tables
This already happens later, see FinalizeEntry
|
Thanks, I really wasn't sure what purpose the |
| auto stats = IcebergPredicateStats(); | ||
| bool found_parition_field = false; | ||
| for (auto &partition_val : file.partition_values) { | ||
| auto source_id = partition_val.first; |
There was a problem hiding this comment.
This shouldn't be called source_id, the partition values are matched on the field_id of the partition:
Partition data tuple, schema based on the partition spec output using partition field ids for the struct field ids
|
|
||
| // initialize dummy stats | ||
| auto stats = IcebergPredicateStats(); | ||
| bool found_parition_field = false; |
| } | ||
|
|
||
| if (!found_parition_field) { | ||
| break; |
There was a problem hiding this comment.
I'm not sure if breaking or continuing is the best course of action here, can't we still determine if a file should be skipped based on the other partitions that are present?
| break; | ||
| } | ||
|
|
||
| auto result_type = field.transform.GetSerializedType(column.type); |
There was a problem hiding this comment.
I went looking to see if the partition values adhere to the same schema evolution rules as lower/upper bounds of a data file, but they do not:
Type promotion is not allowed for a field that is referenced by source-id or source-ids of a partition field if the partition transform would produce a different value after promoting the type
So this is correct 👍
There was a problem hiding this comment.
I went looking at iceberg-python just to check, but this is so abstract..
https://github.com/apache/iceberg-python/blob/main/pyiceberg/table/__init__.py#L1929
I can't make out what they're actually doing 🙃
But then this:
def _build_partition_evaluator(self, spec_id: int) -> Callable[[DataFile], bool]:
spec = self.table_metadata.specs()[spec_id]
partition_type = spec.partition_type(self.table_metadata.schema())
partition_schema = Schema(*partition_type.fields)
partition_expr = self.partition_filters[spec_id]
# The lambda created here is run in multiple threads.
# So we avoid creating _EvaluatorExpression methods bound to a single
# shared instance across multiple threads.
return lambda data_file: expression_evaluator(partition_schema, partition_expr, self.case_sensitive)(data_file.partition)Makes me think that schema evolution is applied, and we need to potentially up-cast the stored partition value to the current type of the partition spec's field ..
There was a problem hiding this comment.
Type promotion is not allowed for a field that is referenced by source-id or source-ids of a partition field if the partition transform would produce a different value after promoting the type
I think I get what is happening here.
If you promote an int column that is partitioned by bucket(a) to string, then the new partition values on strings are different than on ints, (i.e 24 vs "24"). When querying with a filter on the string partitioned column (where a = "24"), DuckDB will assume the constant value is a string, and not an int, and the transform for partition pruning may be incorrect.
For safe type promotions (ie int -> long) existing data files remain valid. To figure out what kind of Transform to apply to the filter constant, you can look at the partition_id in each entry of a manifest_list. Every manifest file has only data files from one partition. Using that partition_id, we can find out what transform was used to write the data files, and apply that to the expression constant to see if we can filter out certain data files/manifest files.
The spec also says that a manifest file can only list data files from the same partition spec. https://iceberg.apache.org/spec/#partitioning
Don't know if this helps at all
There was a problem hiding this comment.
Yea you're right, so schema evolution can happen, as long as it doesn't change the meaning of the partition transform
Then I think this logic needs to up-cast the stored partition Value, don't you think?
There was a problem hiding this comment.
So just to provide an update for future reference:
I don't think any upcasting is necessary
The lower/upper bounds in the data_file are serialized according to the type of the column at the time of file creation.
To properly deserialize them we need to infer what this type was, in case schema evolution took place. (i.e INTEGER -> BIGINT, the bounds are inferred to be INTEGER when the size of the blob is 4 (sizeof(int32_t))
Since the partition values have type information stored, there is no need for any inference.
|
@Tishj Also added some more logic around null partitions, I can also take it out and put it in a separate PR if needed |
Tishj
left a comment
There was a problem hiding this comment.
Thanks, this looks great, the addition of null handling is great as well 👍
|
Thanks, I'm gonna add some more tests though around promoting partitioned types just to make sure. I.e identity, truncate, and void are the only transforms where the resulting type can change |
|
So int -> varchar type promotion is not supported in Iceberg, so we don't have to worry about that. (source) But I will still add a case where a data file cannot be filtered out based on the partition, but it can be filtered out based on upper and lower bounds. |
…rtition matches but not the upper/lower bounds
Given a table with many small files and many snapshots, it's possible some kind of compaction is performed. During this time, new manifest lists and manifest files (and potentially parquet files) are written. Somewhere in the Spark Rest catalog, new manifest files that are written during compaction do not contain upper and lower bound values for certain columns. What spark will do, however, is write partition information in the
partitionsfield of the manifest columns. DuckDB only checks upper and lower bound values of columns in manifest files. Because of this, DuckDB will end up reading waaaaay too many files when there is a filter on a partitioned column.To fix this problem, DuckDB will now read partition information in manifest lists.
Some other notes.