alamb commented on code in PR #16771:
URL: https://github.com/apache/datafusion/pull/16771#discussion_r2205599269
##########
datafusion/expr/src/expr.rs:
##########
@@ -3818,7 +3816,7 @@ mod test {
// If this test fails when you change `Expr`, please try
// `Box`ing the fields to make `Expr` smaller
// See https://github.com/apache/datafusion/issues/16199 for details
- assert_eq!(size_of::<Expr>(), 128);
+ assert_eq!(size_of::<Expr>(), 112);
Review Comment:
This change saves 16 bytes per Expr. Nice.
I will run some planning benchmarks and see if we can see any difference
##########
datafusion/expr/src/expr.rs:
##########
@@ -279,7 +279,7 @@ use sqlparser::ast::{
#[derive(Clone, PartialEq, PartialOrd, Eq, Debug, Hash)]
pub enum Expr {
/// An expression with a specific name.
- Alias(Alias),
+ Alias(Box<Alias>),
Review Comment:
Another change that might be a bit less impactful would be to box the fields
of Alias instead
So Alias {
expr: Box<Expr>
..
}
```
It may be just as bad / worse though
##########
datafusion/sqllogictest/test_files/joins.slt:
##########
@@ -4009,12 +4009,12 @@ logical_plan
09)------------Unnest:
lists[__unnest_placeholder(generate_series(Int64(1),outer_ref(t1.t1_int)))|depth=1]
structs[]
10)--------------Projection: generate_series(Int64(1),
CAST(outer_ref(t1.t1_int) AS Int64)) AS
__unnest_placeholder(generate_series(Int64(1),outer_ref(t1.t1_int)))
11)----------------EmptyRelation
-physical_plan_error This feature is not implemented: Physical plan does not
support logical expression OuterReferenceColumn(UInt32, Column { relation:
Some(Bare { table: "t1" }), name: "t1_int" })
+physical_plan_error This feature is not implemented: Physical plan does not
support logical expression OuterReferenceColumn((UInt32, Column { relation:
Some(Bare { table: "t1" }), name: "t1_int" }))
Review Comment:
do you know why the plan changes? I don't think the extra `()` really adds
much -- can we restore the original?
##########
datafusion/expr/src/expr.rs:
##########
@@ -362,7 +362,7 @@ pub enum Expr {
Placeholder(Placeholder),
/// A placeholder which holds a reference to a qualified field
/// in the outer query, used for correlated sub queries.
- OuterReferenceColumn(DataType, Column),
+ OuterReferenceColumn(Box<(DataType, Column)>),
Review Comment:
If we are going to make this change anyways, can we also pull this into a
named struct rather than a unnamed tuple
like
```rust
struct OuterReference {
// fields here
}
enum Expr {
...
OuterReferenceColumn(OuterReference),
...
}
```
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
