Allow specifying an arrow schema for PartitionedFile#22360
Conversation
github-actions
Bot
added
catalog
There was a problem hiding this comment.
@fpetkovski
Thanks for working on this. I think there is one end-to-end serialization issue that needs to be addressed before this lands.
| pub metadata_size_hint: Option<usize>, | ||
| pub table_reference: Option<TableReference>, | ||
| /// A user-provided arrow schema for the file. | ||
| pub arrow_schema: Option<SchemaRef>, |
There was a problem hiding this comment.
I think this needs a follow-up before merge. PartitionedFile::arrow_schema introduces a new user-provided scan contract, but physical plan proto serialization currently appears to drop it. datafusion/proto/src/physical_plan/to_proto.rs builds protobuf::PartitionedFile without this field, and datafusion/proto/proto/datafusion.proto does not seem to have a schema field for it.
As a result, a Parquet scan that is serialized and deserialized would lose the explicit schema and fall back to parsing ARROW:schema, so the main guarantee from this change would not hold end to end.
Could you please add this field to the proto model and conversions, plus a roundtrip test showing that PartitionedFile::with_arrow_schema(...) survives physical plan or PartitionedFile proto serialization?
There was a problem hiding this comment.
Thanks, I updated the protos to serialize and deserialize the file arrow schema as well. There is a proto test now which verifies the round trip.
| /// The estimated size of the parquet metadata, in bytes | ||
| pub metadata_size_hint: Option<usize>, | ||
| pub table_reference: Option<TableReference>, | ||
| /// A user-provided arrow schema for the file. |
There was a problem hiding this comment.
Small doc suggestion: it would be helpful to make the public contract a bit more precise here. My read is that this is the physical Arrow file schema used by the Parquet opener, it should describe file columns rather than partition columns, and it is currently ignored by non-Parquet sources.
Calling that out explicitly should help avoid users passing a table schema that includes partitions, or expecting CSV and JSON readers to honor this field.
There was a problem hiding this comment.
Thanks for the suggestion, I updated the docs to clarify how this field is used by various openers.
fpetkovski
force-pushed
the
partitioned-file-schema
branch
6 times, most recently
from
1bdaf0e to
bf0c9ab
Compare
fpetkovski
force-pushed
the
partitioned-file-schema
branch
from
7feb2ce to
2c719c8
Compare
2 participants
Which issue does this PR close?
Rationale for this change
As described in the linked issue, parsing the arrow schema from parquet metadata can be expensive for point lookups, relative to the rest of the query execution pipeline. If the user knows the arrow schema of the file, they should be able to specify it explicitly.
What changes are included in this PR?
arrow_schema: SchemaReffield toPartitionedFilearrow_schemafield in the parquet opener to bypass schema inference from theARROW:schemametadata field.Are these changes tested?
Added unit tests for both matching and mismatching schemas.
Are there any user-facing changes?
There are no breaking changes, the new field is optional and is set to None by default.