-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Per file filter evaluation #15057
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Per file filter evaluation #15057
Conversation
|
The example is not working yet. It gets |
|
Ok the example is now working and I think the overall approach is interesting but I don't think it's quite close to a workable solution. |
a21f316 to
3bb4c36
Compare
|
The example is now working and even does stats pruning of shredded columns 🚀 |
| let parquet_source = ParquetSource::default() | ||
| .with_predicate(self.schema.clone(), filter) | ||
| .with_pushdown_filters(true) | ||
| .with_filter_expression_rewriter(Arc::new(StructFieldRewriter) as _); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the API for users to attach this rewriter to their plan
| struct StructFieldRewriterImpl { | ||
| file_schema: SchemaRef, | ||
| } | ||
|
|
||
| impl TreeNodeRewriter for StructFieldRewriterImpl { | ||
| type Node = Arc<dyn PhysicalExpr>; | ||
|
|
||
| fn f_down( | ||
| &mut self, | ||
| expr: Arc<dyn PhysicalExpr>, | ||
| ) -> Result<Transformed<Arc<dyn PhysicalExpr>>> { | ||
| if let Some(scalar_function) = expr.as_any().downcast_ref::<ScalarFunctionExpr>() | ||
| { | ||
| if scalar_function.name() == "get_field" { | ||
| if scalar_function.args().len() == 2 { | ||
| // First argument is the column, second argument is the field name | ||
| let column = scalar_function.args()[0].clone(); | ||
| let field_name = scalar_function.args()[1].clone(); | ||
| if let Some(literal) = | ||
| field_name.as_any().downcast_ref::<expressions::Literal>() | ||
| { | ||
| if let Some(field_name) = literal.value().try_as_str().flatten() { | ||
| if let Some(column) = | ||
| column.as_any().downcast_ref::<expressions::Column>() | ||
| { | ||
| let column_name = column.name(); | ||
| let source_field = | ||
| self.file_schema.field_with_name(column_name)?; | ||
| let expected_flattened_column_name = | ||
| format!("_{}.{}", column_name, field_name); | ||
| // Check if the flattened column exists in the file schema and has the same type | ||
| if let Ok(shredded_field) = self | ||
| .file_schema | ||
| .field_with_name(&expected_flattened_column_name) | ||
| { | ||
| if source_field.data_type() | ||
| == shredded_field.data_type() | ||
| { | ||
| // Rewrite the expression to use the flattened column | ||
| let rewritten_expr = expressions::col( | ||
| &expected_flattened_column_name, | ||
| &self.file_schema, | ||
| )?; | ||
| return Ok(Transformed::yes(rewritten_expr)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(Transformed::no(expr)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example implementation of a rewriter
|
@alamb I think this is ready for a first round of review when you have a chance! |
|
The main issue I've found with this approach is marking filters as datafusion/datafusion/datasource-parquet/src/row_filter.rs Lines 333 to 336 in 9382add
|
|
Okay I think I can answer my own question: https://github.com/pydantic/datafusion/blob/38356998059a2d08113401ea8111f238899ab0b8/datafusion/core/src/datasource/listing/table.rs#L961-L995 Based on this it seems like it's safe to mark filters as exact if they are getting pushed down 😄 |
008eba0 to
1878f59
Compare
|
Okay folks sorry for the churn, I thought this was in a better state than it ended up being. I've now reworked it to minimize the diff and make sure all existing tests pass. I'm going to add tests for the new functionality now to compliment the example. |
| // Note about schemas: we are actually dealing with _4_ different schemas here: | ||
| // - The table schema as defined by the TableProvider. This is what the user sees, what they get when they `SELECT * FROM table`, etc. | ||
| // - The "virtual" file schema: this is the table schema minus any hive partition columns. This is what the file schema is coerced to. | ||
| // - The physical file schema: this is the schema as defined by the parquet file. This is what the parquet file actually contains. | ||
| // - The filter schema: a hybrid of the virtual file schema and the physical file schema. | ||
| // If a filter is rewritten to reference columns that are in the physical file schema but not the virtual file schema, we need to add those columns to the filter schema so that the filter can be evaluated. | ||
| // This schema is generated by taking any columns from the virtual file schema that are referenced by the filter and adding any columns from the physical file schema that are referenced by the filter but not in the virtual file schema. | ||
| // Columns from the virtual file schema are added in the order they appear in the virtual file schema. | ||
| // The columns from the physical file schema are always added to the end of the schema, in the order they appear in the physical file schema. | ||
| // | ||
| // I think it might be wise to do some renaming of parameters where possible, e.g. rename `file_schema` to `table_schema_without_partition_columns` and `physical_file_schema` or something like that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting bit to ponder upon
| /// Rewrite an expressions to take into account this file's particular schema. | ||
| /// This can be used to evaluate expressions against shredded variant columns or columns that pre-compute expressions (e.g. `day(timestamp)`). | ||
| pub trait FileExpressionRewriter: Debug + Send + Sync { | ||
| /// Rewrite an an expression in the context of a file schema. | ||
| fn rewrite( | ||
| &self, | ||
| file_schema: SchemaRef, | ||
| expr: Arc<dyn PhysicalExpr>, | ||
| ) -> Result<Arc<dyn PhysicalExpr>>; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: if users need the table_schema they can bind that inside of TableProvider::scan
|
I will try and give this a look over the next few days |
a310528 to
59ec143
Compare
|
I would like to resume this work. Some thoughts should the rewrite happen via a new trait as I'm currently doing, or should we add a method I suspect the hard bit with this approach will be edge cases: what if a filter cannot adapt itself to the file schema, but we could cast the column to make it work? I'm thinking something like a UDF that only accepts I think @jayzhan211 proposed something similar in https://github.com/apache/datafusion/pull/15685/files#diff-2b3f5563d9441d3303b57e58e804ab07a10d198973eed20e7751b5a20b955e42. @alamb any thoughts? |
This method is too general and it is unclear what we need to do with the provided schema for each PhysicalExpr, it is not a good idea.
I think it is unavoidable we need to cast the columns to be able to evaluate the filter. Another question is, isn't the filter created based on table schema? And then the batch is read as file schema and casted to table schema and is evaluated by filter. What we could do is rewrite the filter based on file schema. Assume we have |
Yes this is exactly the case.
Yes that is exactly what I am proposing above, thank you for giving a more concrete example. The other point is if we can use this same mechanism to handle shredding for the variant type. In other words, can we "optimize" And if that all makes sense... how do we do those optimizations? Is it something like an optimizer that has to downcast match on the expressions, or do we add methods to PhysicalExpr for each expression to describe how it handles this behavior? |
ac3c8d4 to
280fab1
Compare
| ); | ||
|
|
||
| println!("\n=== Demonstrating default value injection in filter predicates ==="); | ||
| let query = "SELECT id, name FROM example_table WHERE status = 'active' ORDER BY id"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alamb this is the example we talked about on the call today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be even more interesting: an example showing generated expression defaults:
ALTER TABLE t ADD COLUMN g DEFAULT (other_col);
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @adriangb -- I think this is looking great. Thank you for sticking with it
I think it would be great too if we could file a ticket to track the consolidation of SchemaAdapter and PhysicalExprRewriter
| // Important: PhysicalExprAdapter is specifically designed for rewriting filter predicates | ||
| // that get pushed down to file scans. For handling missing columns in projections, | ||
| // other mechanisms in DataFusion are used (like SchemaAdapter). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe here would be a good place to leave a link to the ticket describing the unification of SchemaAdapter and PhysicalExprAdapter)
| expr: Arc<dyn PhysicalExpr>, | ||
| logical_file_schema: &Schema, | ||
| physical_file_schema: &Schema, | ||
| partition_values: &[(FieldRef, ScalarValue)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but it seem to me that partition_values shouldn't really be in PhysicalExprAdapter -- rather the ListingTable should provide a PhysicalExprAdapter that knows how to fill in partition values
As a follow on PR perhaps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting idea. I think let's see how the refactoring of FileScanConfigBuilder & co goes, then we can revisit moving the injection of the partition values around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we end up with PhysicalExprAdapterFactory that users create and then gets called as let adapter = physical_expr_adapter_factory.create_adapter(logical_file_schema, physical_file_schema).with_partition_values(...); and let expr = adapter.rewrite(expr);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary for the TableProvider to create it. The FileOpener has the partition values, the logical file schema and the physical file schema. Splitting up initialization will add complexity I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of my thinking behind PhysicalExprAdapterFactory is that when we get around to projection pushdown we'll want to adapt multiple expressions. By having a factory we can (1) minimize boilerplate and (2) have the opportunity to pre-compute e.g. missing columns or other mappings to make the rewrites more performant.
…r API - Update all tests in schema_rewriter.rs to use DefaultPhysicalExprAdapterFactory - Update documentation examples to demonstrate factory pattern - Update default_column_values.rs example to use factory-style API - Convert from rewrite_to_file_schema method to rewrite method with factory pattern - Add proper partition values handling with with_partition_values method 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
I plan on merging #15057 once CI passes. It has been approved / reviewed and we need the customization of the rewriters for #16235 (comment). I will follow up with PRs to:
|
|
Thank you @adriangb |
TableProviders#14993.I decided to tackle filter pushdown first because:
The idea is that we can experiment with filters because it's less work and later re-use
FileExpressionRewriterto do projection pushdown once we flesh out the details and apply learnings from this piece of work.