Copilot commented on code in PR #26879:
URL: https://github.com/apache/datafusion/pull/26879#discussion_r4421830850
##########
datafusion/physical-expr/src/equivalence/properties/dependency.rs:
##########
@@ -1550,4 +1552,211 @@ mod tests {
Ok(())
}
+
+ /// A pass-through expression with its own identity that records the data
+ /// type of the range it is given for its child. It keeps its child's
+ /// ordering, or declares itself ascending when `ordered` is set.
+ #[derive(Debug)]
+ struct RangeProbe {
+ child: Arc<dyn PhysicalExpr>,
+ id: usize,
+ ordered: bool,
+ seen: Arc<std::sync::Mutex<Vec<DataType>>>,
+ }
+
+ impl PartialEq for RangeProbe {
+ fn eq(&self, other: &Self) -> bool {
+ self.id == other.id && self.child.eq(&other.child)
+ }
+ }
+ impl Eq for RangeProbe {}
+ impl std::hash::Hash for RangeProbe {
+ fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+ self.id.hash(state);
+ self.child.hash(state);
+ }
+ }
+ impl Display for RangeProbe {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "probe{}({})", self.id, self.child)
+ }
+ }
+
+ impl PhysicalExpr for RangeProbe {
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.child.data_type(input_schema)
+ }
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.child.nullable(input_schema)
+ }
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ self.child.evaluate(batch)
+ }
+ fn children(&self) -> Vec<&Arc<dyn PhysicalExpr>> {
+ vec![&self.child]
+ }
+ fn with_new_children(
+ self: Arc<Self>,
+ children: Vec<Arc<dyn PhysicalExpr>>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ Ok(Arc::new(RangeProbe {
+ child: Arc::clone(&children[0]),
+ id: self.id,
+ ordered: self.ordered,
+ seen: Arc::clone(&self.seen),
+ }))
+ }
Review Comment:
`with_new_children` unconditionally indexes `children[0]`, which will panic
if called with an unexpected number of children. Return an error when
`children.len() != 1` (or use `children.into_iter().next()` with proper
validation) to keep the `PhysicalExpr` implementation robust.
##########
datafusion/physical-expr/src/equivalence/properties/mod.rs:
##########
@@ -1599,6 +1611,34 @@ fn expr_refers(
.any(|child| expr_refers(child, referred_expr))
}
+/// Unbounded ranges of the columns of one schema, built on first use.
+///
+/// Building the unbounded range of a nested type materializes a null array of
that type, so
+/// a wide struct referenced by many projection sources is costly to rebuild
per reference.
+/// Only `Column`s are cached, by index: the schema is fixed for the cache's
lifetime. Other
+/// expressions and failures are computed as before and never cached. Cloning
a cached range
+/// clones its two endpoints, which for nested types share their arrays.
+#[derive(Default)]
+struct ColumnRanges(std::cell::RefCell<HashMap<usize, Interval>>);
+
+impl ColumnRanges {
+ fn unbounded(
+ &self,
+ expr: &Arc<dyn PhysicalExpr>,
+ schema: &SchemaRef,
+ ) -> Result<Interval> {
+ let Some(column) = expr.downcast_ref::<Column>() else {
+ return Interval::make_unbounded(&expr.data_type(schema)?);
+ };
+ if let Some(range) = self.0.borrow().get(&column.index()) {
+ return Ok(range.clone());
+ }
+ let range = Interval::make_unbounded(&expr.data_type(schema)?)?;
+ self.0.borrow_mut().insert(column.index(), range.clone());
+ Ok(range)
+ }
+}
Review Comment:
Since the cache key is a column index within a fixed schema, using a
`HashMap` adds hashing overhead and extra allocations/indirections on the hot
path. Consider switching to a `Vec<Option<Interval>>` (sized to
`schema.fields().len()`) for O(1) indexing without hashing; it also avoids
repeated `column.index()` lookups and can reduce borrow traffic.
##########
datafusion/physical-expr/src/equivalence/properties/dependency.rs:
##########
@@ -1550,4 +1552,211 @@ mod tests {
Ok(())
}
+
+ /// A pass-through expression with its own identity that records the data
+ /// type of the range it is given for its child. It keeps its child's
+ /// ordering, or declares itself ascending when `ordered` is set.
+ #[derive(Debug)]
+ struct RangeProbe {
+ child: Arc<dyn PhysicalExpr>,
+ id: usize,
+ ordered: bool,
+ seen: Arc<std::sync::Mutex<Vec<DataType>>>,
+ }
+
+ impl PartialEq for RangeProbe {
+ fn eq(&self, other: &Self) -> bool {
+ self.id == other.id && self.child.eq(&other.child)
+ }
+ }
+ impl Eq for RangeProbe {}
+ impl std::hash::Hash for RangeProbe {
+ fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+ self.id.hash(state);
+ self.child.hash(state);
+ }
+ }
+ impl Display for RangeProbe {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "probe{}({})", self.id, self.child)
+ }
+ }
+
+ impl PhysicalExpr for RangeProbe {
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
+ self.child.data_type(input_schema)
+ }
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
+ self.child.nullable(input_schema)
+ }
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+ self.child.evaluate(batch)
+ }
+ fn children(&self) -> Vec<&Arc<dyn PhysicalExpr>> {
+ vec![&self.child]
+ }
+ fn with_new_children(
+ self: Arc<Self>,
+ children: Vec<Arc<dyn PhysicalExpr>>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ Ok(Arc::new(RangeProbe {
+ child: Arc::clone(&children[0]),
+ id: self.id,
+ ordered: self.ordered,
+ seen: Arc::clone(&self.seen),
+ }))
+ }
+ fn get_properties(&self, children: &[ExprProperties]) ->
Result<ExprProperties> {
+ self.seen
+ .lock()
+ .unwrap()
+ .push(children[0].range.data_type());
+ let order = if self.ordered {
+ SortProperties::Ordered(SortOptions::default())
+ } else {
+ children[0].sort_properties
+ };
+ Ok(ExprProperties::new_unknown().with_order(order))
Review Comment:
The probe is described as a pass-through expression, but it returns
`ExprProperties::new_unknown()` which discards the child’s range (and other
derived properties). To make the tests more future-proof (e.g., if range starts
influencing ordering derivation later), consider propagating the child’s
`range`/properties and only overriding the `sort_properties` as needed.
##########
datafusion/physical-expr/src/equivalence/properties/mod.rs:
##########
@@ -233,7 +233,9 @@ impl EquivalenceProperties {
return None;
}
let dependencies =
Dependencies::new(std::iter::once(sort_expr.clone()));
- let properties = get_expr_properties(&r_expr, &dependencies,
schema).ok()?;
+ let properties =
+ get_expr_properties(&r_expr, &dependencies, schema,
&ColumnRanges::default())
+ .ok()?;
Review Comment:
Passing `&ColumnRanges::default()` creates a temporary inline, which is
correct but a bit harder to read and makes it easier to accidentally duplicate
cache creation in future edits. Prefer binding a local `let column_ranges =
ColumnRanges::default();` and passing `&column_ranges` for clarity and
consistency with the `projected_orderings` call site.
--
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]
