Skip to content

Commit 998950a

Browse files
zhuqi-lucaszhuqi-lucas
committed
address comments
1 parent 38b87bf commit 998950a

File tree

13 files changed

+69
-36
lines changed

13 files changed

+69
-36
lines changed

‎datafusion/catalog-listing/src/helpers.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ pub fn expr_applicable_for_cols(col_names: &[&str], expr: &Expr) -> bool {
6363
}
6464
Expr::Literal(_, _)
6565
| Expr::Alias(_)
66-
| Expr::OuterReferenceColumn(_, _)
66+
| Expr::OuterReferenceColumn(_)
6767
| Expr::ScalarVariable(_, _)
6868
| Expr::Not(_)
6969
| Expr::IsNotNull(_)

‎datafusion/expr/src/expr.rs‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
2020
use std::cmp::Ordering;
2121
use std::collections::{BTreeMap, HashSet};
22-
use std::fmt::{self, Display, Formatter, Write};
22+
use std::fmt::{self, Debug, Display, Formatter, Write};
2323
use std::hash::{Hash, Hasher};
2424
use std::mem;
2525
use std::sync::Arc;
@@ -362,7 +362,7 @@ pub enum Expr {
362362
Placeholder(Placeholder),
363363
/// A placeholder which holds a reference to a qualified field
364364
/// in the outer query, used for correlated sub queries.
365-
OuterReferenceColumn(DataType, Column),
365+
OuterReferenceColumn(Box<OuterReference>),
366366
/// Unnest expression
367367
Unnest(Unnest),
368368
}
@@ -413,6 +413,18 @@ impl<'a> TreeNodeContainer<'a, Self> for Expr {
413413
}
414414
}
415415

416+
#[derive(Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
417+
pub struct OuterReference {
418+
pub data_type: DataType,
419+
pub column: Column,
420+
}
421+
422+
impl Debug for OuterReference {
423+
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
424+
write!(f, "{:?}, {:?}", self.data_type, self.column)
425+
}
426+
}
427+
416428
/// Literal metadata
417429
///
418430
/// Stores metadata associated with a literal expressions
@@ -1443,7 +1455,7 @@ impl Expr {
14431455
Expr::Case { .. } => "Case",
14441456
Expr::Cast { .. } => "Cast",
14451457
Expr::Column(..) => "Column",
1446-
Expr::OuterReferenceColumn(_, _) => "Outer",
1458+
Expr::OuterReferenceColumn(_) => "Outer",
14471459
Expr::Exists { .. } => "Exists",
14481460
Expr::GroupingSet(..) => "GroupingSet",
14491461
Expr::InList { .. } => "InList",
@@ -2018,7 +2030,7 @@ impl Expr {
20182030
| Expr::SimilarTo(..)
20192031
| Expr::Not(..)
20202032
| Expr::Negative(..)
2021-
| Expr::OuterReferenceColumn(_, _)
2033+
| Expr::OuterReferenceColumn(_)
20222034
| Expr::TryCast(..)
20232035
| Expr::Unnest(..)
20242036
| Expr::Wildcard { .. }
@@ -2601,9 +2613,9 @@ impl HashNode for Expr {
26012613
Expr::Placeholder(place_holder) => {
26022614
place_holder.hash(state);
26032615
}
2604-
Expr::OuterReferenceColumn(data_type, column) => {
2605-
data_type.hash(state);
2606-
column.hash(state);
2616+
Expr::OuterReferenceColumn(boxed_orc) => {
2617+
boxed_orc.data_type.hash(state);
2618+
boxed_orc.column.hash(state);
26072619
}
26082620
Expr::Unnest(Unnest { expr: _expr }) => {}
26092621
};
@@ -2908,7 +2920,7 @@ struct SqlDisplay<'a>(&'a Expr);
29082920
impl Display for SqlDisplay<'_> {
29092921
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
29102922
match self.0 {
2911-
Expr::Literal(scalar, _) => scalar.fmt(f),
2923+
Expr::Literal(scalar, _) => Display::fmt(&scalar, f),
29122924
Expr::Alias(Alias { name, .. }) => write!(f, "{name}"),
29132925
Expr::Between(Between {
29142926
expr,
@@ -3171,7 +3183,8 @@ impl Display for Expr {
31713183
match self {
31723184
Expr::Alias(Alias { expr, name, .. }) => write!(f, "{expr} AS {name}"),
31733185
Expr::Column(c) => write!(f, "{c}"),
3174-
Expr::OuterReferenceColumn(_, c) => {
3186+
Expr::OuterReferenceColumn(boxed_orc) => {
3187+
let c = &boxed_orc.column;
31753188
write!(f, "{OUTER_REFERENCE_COLUMN_PREFIX}({c})")
31763189
}
31773190
Expr::ScalarVariable(_, var_names) => write!(f, "{}", var_names.join(".")),
@@ -3818,7 +3831,7 @@ mod test {
38183831
// If this test fails when you change `Expr`, please try
38193832
// `Box`ing the fields to make `Expr` smaller
38203833
// See https://github.com/apache/datafusion/issues/16199 for details
3821-
assert_eq!(size_of::<Expr>(), 128);
3834+
assert_eq!(size_of::<Expr>(), 112);
38223835
assert_eq!(size_of::<ScalarValue>(), 64);
38233836
assert_eq!(size_of::<DataType>(), 24); // 3 ptrs
38243837
assert_eq!(size_of::<Vec<Expr>>(), 24);

‎datafusion/expr/src/expr_fn.rs‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@
1919
2020
use crate::expr::{
2121
AggregateFunction, BinaryExpr, Cast, Exists, GroupingSet, InList, InSubquery,
22-
Placeholder, TryCast, Unnest, WildcardOptions, WindowFunction, WindowFunctionParams,
22+
OuterReference, Placeholder, TryCast, Unnest, WildcardOptions, WindowFunction,
23+
WindowFunctionParams,
2324
};
2425
use crate::function::{
2526
AccumulatorArgs, AccumulatorFactoryFunction, PartitionEvaluatorFactory,
@@ -69,7 +70,13 @@ pub fn col(ident: impl Into<Column>) -> Expr {
6970
/// Create an out reference column which hold a reference that has been resolved to a field
7071
/// outside of the current plan.
7172
pub fn out_ref_col(dt: DataType, ident: impl Into<Column>) -> Expr {
72-
Expr::OuterReferenceColumn(dt, ident.into())
73+
Expr::OuterReferenceColumn(
74+
OuterReference {
75+
data_type: dt,
76+
column: ident.into(),
77+
}
78+
.into(),
79+
)
7380
}
7481

7582
/// Create an unqualified column expression from the provided name, without normalizing

‎datafusion/expr/src/expr_rewriter/mod.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,8 +200,9 @@ pub fn unnormalize_cols(exprs: impl IntoIterator<Item = Expr>) -> Vec<Expr> {
200200
pub fn strip_outer_reference(expr: Expr) -> Expr {
201201
expr.transform(|expr| {
202202
Ok({
203-
if let Expr::OuterReferenceColumn(_, col) = expr {
204-
Transformed::yes(Expr::Column(col))
203+
// Match the boxed (DataType, Column) tuple and extract the Column
204+
if let Expr::OuterReferenceColumn(boxed_pair) = expr {
205+
Transformed::yes(Expr::Column(boxed_pair.column))
205206
} else {
206207
Transformed::no(expr)
207208
}

‎datafusion/expr/src/expr_schema.rs‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,9 @@ impl ExprSchemable for Expr {
112112
},
113113
Expr::Negative(expr) => expr.get_type(schema),
114114
Expr::Column(c) => Ok(schema.data_type(c)?.clone()),
115-
Expr::OuterReferenceColumn(ty, _) => Ok(ty.clone()),
115+
Expr::OuterReferenceColumn(boxed_orc) => {
116+
Ok(boxed_orc.as_ref().data_type.clone())
117+
}
116118
Expr::ScalarVariable(ty, _) => Ok(ty.clone()),
117119
Expr::Literal(l, _) => Ok(l.data_type()),
118120
Expr::Case(case) => {
@@ -276,7 +278,7 @@ impl ExprSchemable for Expr {
276278
|| high.nullable(input_schema)?),
277279

278280
Expr::Column(c) => input_schema.nullable(c),
279-
Expr::OuterReferenceColumn(_, _) => Ok(true),
281+
Expr::OuterReferenceColumn(_) => Ok(true),
280282
Expr::Literal(value, _) => Ok(value.is_null()),
281283
Expr::Case(case) => {
282284
// This expression is nullable if any of the input expressions are nullable
@@ -411,9 +413,11 @@ impl ExprSchemable for Expr {
411413
}
412414
Expr::Negative(expr) => expr.to_field(schema).map(|(_, f)| f),
413415
Expr::Column(c) => schema.field_from_column(c).map(|f| Arc::new(f.clone())),
414-
Expr::OuterReferenceColumn(ty, _) => {
415-
Ok(Arc::new(Field::new(&schema_name, ty.clone(), true)))
416-
}
416+
Expr::OuterReferenceColumn(boxed_alias) => Ok(Arc::new(Field::new(
417+
&schema_name,
418+
boxed_alias.as_ref().data_type.clone(),
419+
true,
420+
))),
417421
Expr::ScalarVariable(ty, _) => {
418422
Ok(Arc::new(Field::new(&schema_name, ty.clone(), true)))
419423
}

‎datafusion/expr/src/tree_node.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ impl TreeNode for Expr {
7171
#[expect(deprecated)]
7272
Expr::Column(_)
7373
// Treat OuterReferenceColumn as a leaf expression
74-
| Expr::OuterReferenceColumn(_, _)
74+
| Expr::OuterReferenceColumn(_)
7575
| Expr::ScalarVariable(_, _)
7676
| Expr::Literal(_, _)
7777
| Expr::Exists { .. }
@@ -122,7 +122,7 @@ impl TreeNode for Expr {
122122
Expr::Column(_)
123123
| Expr::Wildcard { .. }
124124
| Expr::Placeholder(Placeholder { .. })
125-
| Expr::OuterReferenceColumn(_, _)
125+
| Expr::OuterReferenceColumn(_)
126126
| Expr::Exists { .. }
127127
| Expr::ScalarSubquery(_)
128128
| Expr::ScalarVariable(_, _)

‎datafusion/optimizer/src/analyzer/type_coercion.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -589,7 +589,7 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
589589
| Expr::Wildcard { .. }
590590
| Expr::GroupingSet(_)
591591
| Expr::Placeholder(_)
592-
| Expr::OuterReferenceColumn(_, _) => Ok(Transformed::no(expr)),
592+
| Expr::OuterReferenceColumn(_) => Ok(Transformed::no(expr)),
593593
}
594594
}
595595
}

‎datafusion/optimizer/src/optimize_projections/mod.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -637,8 +637,8 @@ fn outer_columns<'a>(expr: &'a Expr, columns: &mut HashSet<&'a Column>) {
637637
// inspect_expr_pre doesn't handle subquery references, so find them explicitly
638638
expr.apply(|expr| {
639639
match expr {
640-
Expr::OuterReferenceColumn(_, col) => {
641-
columns.insert(col);
640+
Expr::OuterReferenceColumn(boxed_orc) => {
641+
columns.insert(&boxed_orc.as_ref().column);
642642
}
643643
Expr::ScalarSubquery(subquery) => {
644644
outer_columns_helper_multi(&subquery.outer_ref_columns, columns);

‎datafusion/optimizer/src/push_down_filter.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ fn can_evaluate_as_join_condition(predicate: &Expr) -> Result<bool> {
262262
Expr::Exists { .. }
263263
| Expr::InSubquery(_)
264264
| Expr::ScalarSubquery(_)
265-
| Expr::OuterReferenceColumn(_, _)
265+
| Expr::OuterReferenceColumn(_)
266266
| Expr::Unnest(_) => {
267267
is_evaluate = false;
268268
Ok(TreeNodeRecursion::Stop)

‎datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -628,7 +628,7 @@ impl<'a> ConstEvaluator<'a> {
628628
Expr::AggregateFunction { .. }
629629
| Expr::ScalarVariable(_, _)
630630
| Expr::Column(_)
631-
| Expr::OuterReferenceColumn(_, _)
631+
| Expr::OuterReferenceColumn(_)
632632
| Expr::Exists { .. }
633633
| Expr::InSubquery(_)
634634
| Expr::ScalarSubquery(_)

0 commit comments

Comments
 (0)