mbutrovich commented on code in PR #22026:
URL: https://github.com/apache/datafusion/pull/22026#discussion_r3227209272
##########
datafusion/datasource-parquet/src/opener.rs:
##########
@@ -1323,6 +1464,65 @@ impl PushDecoderStreamState {
type ConstantColumns = HashMap<String, ScalarValue>;
+/// Return `base` unchanged when `extra` is empty; otherwise build a new schema
+/// with `extra` appended to `base`'s fields.
+fn append_fields(base: &SchemaRef, extra: &[FieldRef]) -> SchemaRef {
+ if extra.is_empty() {
+ return Arc::clone(base);
+ }
+ let fields = base
+ .fields()
+ .iter()
+ .cloned()
+ .chain(extra.iter().cloned())
+ .collect::<Vec<_>>();
+ Arc::new(Schema::new(fields))
+}
+
+/// Reject predicates that reference a virtual column when filter pushdown is
+/// enabled.
+///
+/// arrow-rs's `RowFilter` evaluates predicates against a `ProjectionMask` that
+/// addresses parquet leaves only; virtual columns (e.g. `row_number`) are
+/// synthesized by the reader *after* filter evaluation and cannot be
referenced
+/// inside a row filter. Silently dropping such a predicate would produce wrong
+/// results, so we fail loudly here.
+///
+/// `ParquetSource::try_pushdown_filters` already classifies virtual-column
+/// filters as `PushedDown::No` so the `FilterPushdown` optimizer leaves them
+/// above the scan; this check is defense-in-depth for callers that build plans
+/// manually and set `with_pushdown_filters(true)` alongside a predicate
+/// referencing virtual columns.
+fn validate_predicate_does_not_reference_virtual_columns(
Review Comment:
I think the middle-ground I would like: we enforce the contract in a debug
assertion in the opener, so it's not on the critical path of release builds,
but developers who are building on top of DataFusion (particularly in dev
builds or CI builds with debug assertions enabled) will get an error. Not
having that feels like too much of a footgun to me, since we don't have a good
way of enforcing the contract that `try_pushdown_filters` was called first.
What do you think @adriangb?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
