Repository navigation
Add VariantExprAdapterFactory to read files with a different shredding layout - #149
Conversation
alamb
left a comment
There was a problem hiding this comment.
Thank you for this @raghuvanshraj
Could you please add .slt tests showing the usecase ?
This morning we merged the ability to create shredded variants directly in SQL
So it should now be straightforward to setup your usecase for the expression adapter with slt
Here are some docs that explain how shredding works in more detail
| +---------+-----------+ | ||
| ``` | ||
|
|
||
| #### Files with different shredding layouts |
There was a problem hiding this comment.
Should we suggest people install this adapter for all tables that read variant, even for those that aren't shredded, right?
Also, it would be really nice to be able to set this up fr all tables automatically somehow in register_all -- maybe that needs a new datafusion API 🤔
There was a problem hiding this comment.
Should we suggest people install this adapter for all tables that read variant, even for those that aren't shredded, right?
Yes @alamb agreed on this. Anyway an unshredded file never pays anything since the adapter never enters rewrite for those.
There was a problem hiding this comment.
Also, it would be really nice to be able to set this up fr all tables automatically somehow in register_all -- maybe that needs a new datafusion API 🤔
For CREATE EXTERNAL TABLE actually this might work without a new API: register_all can replace the session's PARQUET table factory with one that wraps ListingTableFactory and rebuilds the ListingTable with the adapter attached. Every Parquet table created by SQL in that session then gets it.
@alamb thoughts?
There was a problem hiding this comment.
I think it is a great idea -- I actually tried it locally and it works great. I pushed to this PR and am going to push a few more commits too
6c92645 to
2a608e6
Compare
@alamb I've added |
alamb
left a comment
There was a problem hiding this comment.
Thanks @raghuvanshraj -- this is great. I am going to spend some time pushing some comment cleanups, etc and hopefully we'll have this one ready to go
|
|
||
| /// Register all Variant scalar, aggregate, and table functions, cast rewrites, | ||
| /// `:` field access, SQL `VARIANT` type syntax, and JSON display of Variant values. | ||
| use datafusion::error::Result; |
alamb
left a comment
There was a problem hiding this comment.
Thank you @raghuvanshraj -- I think this is a great step and works as advertised
I went through this PR carefully, and took the liberty of reworking a bunch of the examples, tests and comments in this PR so they are easier to understand.
I am going to go through the tests one more time
After this, I think the next step would be to make a benchmark:
We also now have enough pieces to benchmark reasonably and I have already found at least one problem
|
I'd like to take a pass too |
|
Then again we can merge and iterate |
|
I also filed a follow on issue here |
Sounds great. I will let you merge it then. I don't think there is any rush. I have to work on some other stuff now today, but I think I filed a bunch of tickets and we are pushing this along well |
sdf-jkl
left a comment
There was a problem hiding this comment.
Thanks @raghuvanshraj @alamb -- this is epic 🔥
Adds
VariantExprAdapterFactory, aPhysicalExprAdapterFactorythat wraps DataFusion's default adapter. When a Variant column's layout in a Parquet file differs from the merged table layout, the default adapter casts the file struct to the table struct and null-fills the missingtyped_valuechildren, which the shredding spec reads as "not an object"; every shredded field of that file then returns NULL with no error. The new adapter instead reads the file column as is and, per batch, unshreds it and shreds it again to the table'styped_valueschema. Files whose layout already matches the table are left to the default adapter.Closes #148.
Installed per table with
ListingTableConfig::with_expr_adapter_factory(Arc::new(VariantExprAdapterFactory::default())); the README has the full snippet. Wiring it in automatically for the extension type is a core question, raised on apache/datafusion#27089.Two limits worth knowing: files that need the rewrite read the whole Variant column, so they don't get leaf-level projection; and only whole-column reads of the Variant are rewritten, so direct struct access like
event['typed_value']on a mismatched file still sees the null-filled cast.Tests write real Parquet files and read them through a
ListingTable: a shredded plus an unshredded file (3 of 6 rows NULL without the adapter, 6 of 6 with it), two files shredded on different fields, a whole-column read, and same-layout files showing no rewrite in the plan.Validation:
cargo test(new and existing tests, SLTs, README examples),cargo fmt --check,cargo clippy --all-targets -- -D warnings.