Correct physical schema inference for DF files with non-logical types. #5976
+356
−40
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AdamGS
force-pushed
the
adamg/correct-file-physical-type-in-df
branch
from
4f1074e to
86a6685
Compare
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
Signed-off-by: Alfonso Subiotto Marques <[email protected]>
Signed-off-by: Adam Gutglick <[email protected]>
AdamGS
force-pushed
the
adamg/correct-file-physical-type-in-df
branch
from
86a6685 to
5f66406
Compare
Signed-off-by: Adam Gutglick <[email protected]>
Contributor
Benchmarks: FineWeb NVMeSummary
Detailed Results Table
|
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
asubiotto
reviewed
Jan 16, 2026
| /// | ||
| /// For these types, we use the logical schema's type instead of the DType's natural Arrow | ||
| /// conversion, since Vortex's Arrow executor can produce these types when requested. | ||
| fn calculate_physical_file_schema( |
Contributor
There was a problem hiding this comment.
I think we should remove this. See my comments on the callsites.
| let physical_file_schema = Arc::new(vxf.dtype().to_arrow_schema().map_err(|e| { | ||
| exec_datafusion_err!("Failed to convert file schema to arrow: {e}") | ||
| })?); | ||
| let physical_file_schema = Arc::new(calculate_physical_file_schema( |
Contributor
There was a problem hiding this comment.
I think the nomenclature here is a bit confusing because datafusion's classic interpretation of logical (i.e. table types) vs physical (i.e. file types) doesn't really make sense in a vortex world, where the output arrow types can be dynamic, not static. I think s/physical_file_schema/this_file_schema and s/logical_file_schema/unified_file_schema would make more sense. What do you think?
Also, given the vortex dynamic output schema nature, calculating this_file_schema should just be the subset of unified_file_schema that the file actually stores, no extra type calculations necessary.
Contributor
There was a problem hiding this comment.
The output Arrow types that Vortex reports are always going to be the simple types, e.g. Dictionary and RunEnd will never be reported as a column type even if Vortex encodes it that way.
So I think we need to keep the calculation?
Also I agree that I find DF's nomenclature confusing here. In my head, I always call this the "reader schema" (logical file schema) and the "writer schema" (physical file schema).
If we don't do the calculation here, then by default we never pushdown the cast to Dictionary/RunEnd and always end up doing it in Arrow after.
Contributor
There was a problem hiding this comment.
Right, but what I am suggesting is removing any reference to vxf.dtype() here. The “calculation” would become just finding the subset of columns in logical_file_schema (physical arrow types) that this specific file stores. This should all be done by name.
The complexity of the current calculation that chooses the physical arrow type from one schema or the other based on the type itself is the part that is not necessary I think especially since the physical arrow types from the logical_file_schema are what is “desired” by datafusion and what you need to end up outputting (minus projections) any way.
Contributor
Author
There was a problem hiding this comment.
this conversation split up a bit, but I do think its important to keep pulling from vxf.dtype() and use the names, that's how the expression adapter rewrites the expression to take advantage of the data's concrete structure (in the case of filters like non_exist_col > 5)
| exec_datafusion_err!("Couldn't get the schema for the underlying Vortex scan") | ||
| })?; | ||
| let stream_schema = | ||
| calculate_physical_file_schema(&scan_dtype, &projected_physical_schema)?; |
Contributor
There was a problem hiding this comment.
I think this will be brittle because it's a lookup by name which can be changed with aliases etc... What about retaining the physical arrow type information when converting expressions in split_projection? You could then get a post-scan-projection arrow schema by calling something on the ProcessedProjection.
Contributor
Author
There was a problem hiding this comment.
I agree that its brittle, but it should also be able to maintain the names as both (should) be projected in the same way. I'm thinking through the comments, would definitely love to make this more robust.
Contributor
Author
There was a problem hiding this comment.
That would be a very similar flow right? Just only available after splitting the projection which should make sure all the names match.
Contributor
There was a problem hiding this comment.
Yes, the flow would be pretty much exactly the same, just that every DF expression you successfully convert to vortex would also keep the DF result type, i.e. you calculate the physical arrow schema in the split_projection recursion. I'll defer to you on what to do, but I think we can avoid the complicated calculate_physical_file_schema this way.
|
|
||
| assert_eq!(filtered_scan[0].schema(), read_schema); | ||
|
|
||
| assert_batches_eq!( |
Contributor
There was a problem hiding this comment.
I think this needs to be assert_batches_sorted_eq! as well
Signed-off-by: Adam Gutglick <[email protected]>
Contributor
|
We also would need to add |
Contributor
I don't think so, direct conversion is taken care of by vortex/vortex-array/src/arrow/executor/mod.rs Line 122 in cc81639 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes how a file or a scan's
DTypeis mapped into an arrow types in cases where the table has non-logical types that don't round trip between the types systems.This change both simplifies how we push projection expressions and allows us to save on unnecessary work converting arrays between various logically-equivalent types.