vbarua commented on code in PR #23488:
URL: https://github.com/apache/datafusion/pull/23488#discussion_r3583567189
##########
datafusion/substrait/src/logical_plan/producer/expr/field_reference.rs:
##########
@@ -76,20 +85,37 @@ pub(crate) fn try_to_substrait_field_reference(
}
}
-/// Convert an outer reference column to a Substrait field reference.
-/// Outer reference columns reference columns from an outer query scope in
correlated subqueries.
-/// We convert them the same way as regular columns since the subquery plan
will be
-/// reconstructed with the proper schema context during consumption.
+/// Convert an outer reference column to a Substrait field reference with an
+/// `OuterReference` root type.
+///
+/// Outer reference columns reference columns from an enclosing query scope in
+/// correlated subqueries. The column is resolved against the producer's stack
+/// of outer schemas (pushed at each subquery boundary), innermost first, and
+/// the resulting `steps_out` records how many query boundaries the reference
+/// crosses (`steps_out = 1` is the immediately enclosing query).
pub fn from_outer_reference_column(
+ producer: &mut impl SubstraitProducer,
+ _field: &FieldRef,
Review Comment:
What do we need `_field` for?
##########
datafusion/substrait/src/logical_plan/producer/substrait_producer.rs:
##########
@@ -441,6 +450,30 @@ pub trait SubstraitProducer: Send + Sync + Sized {
from_placeholder(self, placeholder)
}
+ // Outer Schema management API.
+ //
+ // These methods manage a stack of outer schemas for correlated subquery
support
+ // such as when entering a subquery, the enclosing query's schema is
pushed onto
+ // the stack.
+ //
+ // Serializing an Expr::OuterReferenceColumn uses these to resolve the
column
+ // against the correct enclosing query and emit an OuterReference field
reference
+ // with the corresponding `steps_out`.
+
+ /// Push an outer schema onto the stack when entering a subquery.
+ fn push_outer_schema(&mut self, _schema: DFSchemaRef) {}
+
+ /// Pop an outer schema from the stack when leaving a subquery.
+ fn pop_outer_schema(&mut self) {}
+
+ /// Get the outer schema at the given nesting depth.
+ /// `steps_out = 1` is the immediately enclosing query, `steps_out = 2`
+ /// is two levels out, etc. Returns `None` if `steps_out` is 0 or
+ /// exceeds the current nesting depth.
+ fn get_outer_schema(&self, _steps_out: usize) -> Option<DFSchemaRef> {
Review Comment:
minor/opinionated: `get_outer_schema` -> `outer_schema` to better match
`lamba_variable` (no get)
##########
datafusion/substrait/src/logical_plan/producer/expr/field_reference.rs:
##########
@@ -76,20 +85,37 @@ pub(crate) fn try_to_substrait_field_reference(
}
}
-/// Convert an outer reference column to a Substrait field reference.
-/// Outer reference columns reference columns from an outer query scope in
correlated subqueries.
-/// We convert them the same way as regular columns since the subquery plan
will be
-/// reconstructed with the proper schema context during consumption.
+/// Convert an outer reference column to a Substrait field reference with an
+/// `OuterReference` root type.
+///
+/// Outer reference columns reference columns from an enclosing query scope in
+/// correlated subqueries. The column is resolved against the producer's stack
+/// of outer schemas (pushed at each subquery boundary), innermost first, and
+/// the resulting `steps_out` records how many query boundaries the reference
+/// crosses (`steps_out = 1` is the immediately enclosing query).
pub fn from_outer_reference_column(
+ producer: &mut impl SubstraitProducer,
+ _field: &FieldRef,
col: &Column,
- schema: &DFSchemaRef,
+ _schema: &DFSchemaRef,
) -> datafusion::common::Result<Expression> {
- // OuterReferenceColumn is converted similarly to a regular column
reference.
- // The schema provided should be the schema context in which the outer
reference
- // column appears. During Substrait round-trip, the consumer will
reconstruct
- // the outer reference based on the subquery context.
- let index = schema.index_of_column(col)?;
- substrait_field_ref(index)
+ let mut steps_out = 1;
+ while let Some(outer_schema) = producer.get_outer_schema(steps_out) {
+ if let Some(index) = outer_schema.maybe_index_of_column(col) {
Review Comment:
This effectively searches the innermost schemas outwards searching for the
first instance of a column matching `col`. I can imagine some weirdness if you
have ambiguous references to a column with the same qualified name in multiple
scopes. Does DataFusion enforce any requirements about having these names be
unambiguous in plans?
##########
datafusion/substrait/src/logical_plan/producer/substrait_producer.rs:
##########
@@ -441,6 +450,30 @@ pub trait SubstraitProducer: Send + Sync + Sized {
from_placeholder(self, placeholder)
}
+ // Outer Schema management API.
+ //
+ // These methods manage a stack of outer schemas for correlated subquery
support
+ // such as when entering a subquery, the enclosing query's schema is
pushed onto
+ // the stack.
+ //
+ // Serializing an Expr::OuterReferenceColumn uses these to resolve the
column
+ // against the correct enclosing query and emit an OuterReference field
reference
+ // with the corresponding `steps_out`.
+
+ /// Push an outer schema onto the stack when entering a subquery.
+ fn push_outer_schema(&mut self, _schema: DFSchemaRef) {}
+
+ /// Pop an outer schema from the stack when leaving a subquery.
+ fn pop_outer_schema(&mut self) {}
+
+ /// Get the outer schema at the given nesting depth.
+ /// `steps_out = 1` is the immediately enclosing query, `steps_out = 2`
+ /// is two levels out, etc. Returns `None` if `steps_out` is 0 or
+ /// exceeds the current nesting depth.
+ fn get_outer_schema(&self, _steps_out: usize) -> Option<DFSchemaRef> {
+ None
+ }
Review Comment:
Would it make sense to use `not_impl_err!` for the push, pop and get
functions similar to how it was done for lambdas? This would make custom
producers missing these methods fail more loudly.
##########
datafusion/substrait/src/logical_plan/producer/expr/field_reference.rs:
##########
@@ -76,20 +85,37 @@ pub(crate) fn try_to_substrait_field_reference(
}
}
-/// Convert an outer reference column to a Substrait field reference.
-/// Outer reference columns reference columns from an outer query scope in
correlated subqueries.
-/// We convert them the same way as regular columns since the subquery plan
will be
-/// reconstructed with the proper schema context during consumption.
+/// Convert an outer reference column to a Substrait field reference with an
+/// `OuterReference` root type.
+///
+/// Outer reference columns reference columns from an enclosing query scope in
+/// correlated subqueries. The column is resolved against the producer's stack
+/// of outer schemas (pushed at each subquery boundary), innermost first, and
+/// the resulting `steps_out` records how many query boundaries the reference
+/// crosses (`steps_out = 1` is the immediately enclosing query).
pub fn from_outer_reference_column(
+ producer: &mut impl SubstraitProducer,
+ _field: &FieldRef,
col: &Column,
- schema: &DFSchemaRef,
+ _schema: &DFSchemaRef,
Review Comment:
Do we need `_schema` now that we're retrieving the schema from the outer
reference stack? Given that we're already introducing `producer` as a breaking
change here, it would make sense to remove `_schema` if it's not needed anymore.
--
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]
