Parquet row filter struct access tree#23217
Conversation
There was a problem hiding this comment.
@SubhamSinghal
Thanks!
This LGTM. I have one small non-blocking suggestion for future cleanup.
|
|
||
| let pruned_data_type = prune_struct_type(field.data_type(), field_paths); | ||
| let pruned_data_type = prune_struct_type(field.data_type(), node); | ||
| Arc::new(Field::new( |
There was a problem hiding this comment.
Small future cleanup suggestion: if this internal projected schema is expected to preserve wrapper field metadata for pruned structs, it might be worth copying the metadata when rebuilding the root and intermediate Fields here and in prune_struct_type.
I don't think this PR introduces the issue, and it probably is not scan output visible since the terminal fields are cloned and the decoder projection can restore the requested output schema. This is mostly a future proofing and test coverage suggestion.
SubhamSinghal
force-pushed
the
parquet-row-filter-struct-access-tree
branch
from
cfdce5a to
928c566
Compare
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
@SubhamSinghal
Thanks for the iteration.
Looks 👍 to me
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
This is super exciting! I'd love to run some benchmarks on this to show the improvements. We have two struct benchamrks, but I think neither cover this case:
- benches/parquet_struct_query.rs — filters are all single struct-field predicates (WHERE s['id'] = 5, length(s['value']) > 100). Flat schema s{id, value}. Never exercises multi-conjunct shared-prefix planning.
- benches/parquet_struct_projection.rs — has a nice nested schema (s{inner{large_string, small_int}, extra_string}) but is projection-only: zero WHERE clauses. Doesn't touch the row-filter path at all.
Could you add a new benchmark, ideally as it's own PR so that we can run it on this PR after rebasing, that shows the perf improvement?
| Arc::new( | ||
| Field::new(field.name(), pruned_data_type, field.is_nullable()) | ||
| .with_metadata(field.metadata().clone()), | ||
| ) |
There was a problem hiding this comment.
The .with_metadata() seems like a real bug fix - could we isolate that into it's own PR if possible?
There was a problem hiding this comment.
sure, will raise a new PR with this fix.
| let prefix = std::iter::once(root_name.as_str()) | ||
| .chain(access.field_path.iter().map(|p| p.as_str())) | ||
| .collect::<Vec<_>>(); | ||
| let mut leaf_indices = Vec::with_capacity(access_tree.roots.len() * 2); |
There was a problem hiding this comment.
Is this pre-allocation just a guess, or is it based on something? If it' just a guess, could you help understand how you arrived at this? I tend to prefer to not pre-allocate if there's not a clear benchmark or provable reason to do so. I've been bitten before by excessive pre-allocation causing high memory use.
There was a problem hiding this comment.
Dropped in the latest push. The * 2 was a guess — my rough mental model was "each accessed root contributes ~2 selected leaves on average," but there was no benchmark behind it
3 participants
Which issue does this PR close?
Rationale for this change
Are these changes tested?
Yes
Are there any user-facing changes?
No. This is an internal refactor