You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org> on 2024/05/01 19:11:21 UTC
[PR] remove expr node accumulation [datafusion]
MohamedAbdeen21 opened a new pull request, #10333:
URL: https://github.com/apache/datafusion/pull/10333
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #10280.
Thanks @JasonLi-cn for pointing out the relevant code snippet and the detailed description, saved me some time ❤️
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Making the plans human-readable when common sub expression elimination is applied (check example in ticket and edited tests in this PR).
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Changed tests to reflect the change.
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
Readable plans.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1591327910
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -592,109 +591,51 @@ impl ExprMask {
/// Go through an expression tree and generate identifiers for each subexpression.
///
-/// An identifier contains information of the expression itself and its sub-expression.
-/// This visitor implementation use a stack `visit_stack` to track traversal, which
-/// lets us know when a sub-tree's visiting is finished. When `pre_visit` is called
-/// (traversing to a new node), an `EnterMark` and an `ExprItem` will be pushed into stack.
-/// And try to pop out a `EnterMark` on leaving a node (`f_up()`). All `ExprItem`
-/// before the first `EnterMark` is considered to be sub-tree of the leaving node.
-///
-/// This visitor also records identifier in `id_array`. Makes the following traverse
-/// pass can get the identifier of a node without recalculate it. We assign each node
-/// in the expr tree a series number, start from 1, maintained by `series_number`.
-/// Series number represents the order we left (`f_up()`) a node. Has the property
-/// that child node's series number always smaller than parent's. While `id_array` is
-/// organized in the order we enter (`f_down()`) a node. `node_count` helps us to
-/// get the index of `id_array` for each node.
+/// This visitor also assigns an alias symbol to each expression, to be used if the
+/// expression is getting eliminated, and tracks the frequency and datatype of each expression.
///
/// `Expr` without sub-expr (column, literal etc.) will not have identifier
/// because they should not be recognized as common sub-expr.
struct ExprIdentifierVisitor<'a> {
- // param
+ // expr set to be populated by the visitor
expr_set: &'a mut ExprSet,
/// input schema for the node that we're optimizing, so we can determine the correct datatype
/// for each subexpression
input_schema: DFSchemaRef,
- // inner states
- visit_stack: Vec<VisitRecord>,
- /// increased in fn_down, start from 0.
- node_count: usize,
/// which expression should be skipped?
expr_mask: ExprMask,
}
-/// Record item that used when traversing a expression tree.
-enum VisitRecord {
- /// `usize` is the monotone increasing series number assigned in pre_visit().
- /// Starts from 0. Is used to index the identifier array `id_array` in post_visit().
- EnterMark(usize),
- /// the node's children were skipped => jump to f_up on same node
- JumpMark(usize),
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
-}
-
-impl ExprIdentifierVisitor<'_> {
- /// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
- /// before it.
- fn pop_enter_mark(&mut self) -> (usize, Identifier) {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
- match item {
- VisitRecord::EnterMark(idx) | VisitRecord::JumpMark(idx) => {
- return (idx, desc);
- }
- VisitRecord::ExprItem(id) => {
- desc.push_str(&id);
- }
- }
- }
- unreachable!("Enter mark should paired with node number");
- }
-}
-
impl TreeNodeVisitor for ExprIdentifierVisitor<'_> {
type Node = Expr;
fn f_down(&mut self, expr: &Expr) -> Result<TreeNodeRecursion> {
// related to https://github.com/apache/datafusion/issues/8814
// If the expr contain volatile expression or is a short-circuit expression, skip it.
if expr.short_circuits() || expr.is_volatile()? {
- self.visit_stack
- .push(VisitRecord::JumpMark(self.node_count));
return Ok(TreeNodeRecursion::Jump); // go to f_up
}
- self.visit_stack
- .push(VisitRecord::EnterMark(self.node_count));
- self.node_count += 1;
-
Ok(TreeNodeRecursion::Continue)
}
fn f_up(&mut self, expr: &Expr) -> Result<TreeNodeRecursion> {
- let (_idx, sub_expr_identifier) = self.pop_enter_mark();
-
// skip exprs should not be recognize.
if self.expr_mask.ignores(expr) {
- let curr_expr_identifier = ExprSet::expr_identifier(expr);
- self.visit_stack
- .push(VisitRecord::ExprItem(curr_expr_identifier));
return Ok(TreeNodeRecursion::Continue);
}
- let curr_expr_identifier = ExprSet::expr_identifier(expr);
- let alias_symbol = format!("{curr_expr_identifier}{sub_expr_identifier}");
- self.visit_stack
- .push(VisitRecord::ExprItem(alias_symbol.clone()));
+ let curr_expr_identifier = ExprSet::expr_identifier(expr);
let data_type = expr.get_type(&self.input_schema)?;
+ let alias_symbol = format!("#{{{curr_expr_identifier}}}");
Review Comment:
Love that, this can also help us create the alias on-the-fly instead of passing it inside the expr_set.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1595242837
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -656,24 +656,16 @@ enum VisitRecord {
EnterMark(usize),
/// the node's children were skipped => jump to f_up on same node
JumpMark,
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
/// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
/// before it.
- fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
+ fn pop_enter_mark(&mut self) -> Option<usize> {
Review Comment:
We shoudn't change this part.
The logic that builds up an identifier using `visit_stack` / 3 kinds of `VisitRecord` is neccessary and actually a very clever and way to build up an identifier from the current node and sub-identifiers. (An identifier to be a `String` was not that a clever decision and will be fixed in https://github.com/apache/datafusion/issues/10426, but that's a different issue).
This PR shouldn't change how what an identifier is / how it is built up otherwise we end up with identifier colliding bugs again. The `IdArray`, `ExprStats` and `CommonExprs` datastructures require an dentifier to represent a full expression subtreee. This means that:
```
fn expr_identifier(expr: &Expr) -> Identifier {
format!("#{{{expr}}}")
}
```
would cause bugs as shown in 1. of https://github.com/apache/datafusion/pull/10396.
I.e. if we encountered both `col("a") + col("b")` and `col("a + b")` in the expression list to be CSEd and we used `{expr}` (the non-unique stringified representation) as identifiers then the equal identifier (`"a + b"`) of those 2 different expressions would collide and we counted 2 for the occurance of one of the 2 expressions (and the other expression's count would be lost) resulting wrong CSE.
Please note that currently the identifier of `col("a") + col("b")` is `"{a + b|a|b}"` so it doesn't collide with `col("a + b")`'s identifier: `"a + b"`.
Again, this is hard to test now because of the resolution bug: https://github.com/apache/datafusion/issues/10413.
I.e. if we wrote a test where we have
```
select a + b, "a + b" from (
select 1 as a, 2 as b, 1 as "a + b"
)
```
then currently it gets resolved as
```
select "a + b", "a + b" from (
select 1 as a, 2 as b, 1 as "a + b"
)
```
and this prevents me to create a test case for CSE identifier collision.
(Please note that I'm simplifying the identifier collision exmple as simple columns (`col("a + b")`) are not subject to CSE.)
What this PR can do is to change the aliases (use something else than identifiers) to make the plans more readable.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#issuecomment-2096804679
can we please wait with merging this PR until we sort out the issues of the rule in https://github.com/apache/datafusion/pull/10396
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#issuecomment-2103085218
Revert PR: https://github.com/apache/datafusion/pull/10436
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1595242837
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -656,24 +656,16 @@ enum VisitRecord {
EnterMark(usize),
/// the node's children were skipped => jump to f_up on same node
JumpMark,
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
/// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
/// before it.
- fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
+ fn pop_enter_mark(&mut self) -> Option<usize> {
Review Comment:
We shoudn't change this part.
The logic that builds up an identifier using `visit_stack` / 3 kinds of `VisitRecord` is neccessary and actually a very clever and way to build up an identifier from the current node and sub-identifiers. (An identifier to be a `String` was not that a clever decision and will be fixed in https://github.com/apache/datafusion/issues/10426, but that's a different issue).
This PR shouldn't change what an identifier is / how it is built up otherwise we end up with identifier colliding bugs again. The `IdArray`, `ExprStats` and `CommonExprs` datastructures require an identifier to represent a full expression subtreee. This means that:
```
fn expr_identifier(expr: &Expr) -> Identifier {
format!("#{{{expr}}}")
}
```
would cause bugs as shown in 1. of https://github.com/apache/datafusion/pull/10396.
I.e. if we encountered both `col("a") + col("b")` and `col("a + b")` in the expression list to be CSEd and we used `"{expr}"` (the non-unique stringified representation) as identifiers then the equal identifier (`"a + b"`) of those 2 different expressions would collide and we counted 2 for the occurance of one of the 2 expressions (and the other expression's count would be lost) resulting wrong CSE.
Please note that currently the identifier of `col("a") + col("b")` is `"{a + b|b|a}"` so it doesn't collide with `col("a + b")`'s identifier: `"{a + b}"`.
Again, this is hard to test now because of the resolution bug: https://github.com/apache/datafusion/issues/10413.
I.e. if we write a test where we have
```
select a + b, "a + b" from (
select 1 as a, 2 as b, 1 as "a + b"
)
```
then currently it gets resolved as
```
select "a + b", "a + b" from (
select 1 as a, 2 as b, 1 as "a + b"
)
```
and this prevents us to create a test case for CSE identifier collision.
(Please note that I'm simplifying the identifier collision exmple as simple columns (`col("a + b")`) are not subject to CSE.)
What this PR can do is to change the aliases (use something else than identifiers) to make the plans more readable.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#issuecomment-2094898808
# Benchmark results
<details>
<summary>Benchmarks comparing b412dbab894938d1c3cc732814b7975e47c26df7 (main) and dc5cf09c9d251878a9fedd303e8328559ae5bcfb (PR)</summary>
```
Comparing b412dba and dc5cf09
--------------------
Benchmark tpch_mem_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ b412dba ┃ dc5cf09 ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1 │ 285.83ms │ 288.67ms │ no change │
│ QQuery 2 │ 41.73ms │ 40.62ms │ no change │
│ QQuery 3 │ 58.43ms │ 59.84ms │ no change │
│ QQuery 4 │ 74.13ms │ 82.33ms │ 1.11x slower │
│ QQuery 5 │ 101.34ms │ 103.10ms │ no change │
│ QQuery 6 │ 15.83ms │ 15.85ms │ no change │
│ QQuery 7 │ 235.81ms │ 225.12ms │ no change │
│ QQuery 8 │ 41.77ms │ 40.85ms │ no change │
│ QQuery 9 │ 120.96ms │ 117.94ms │ no change │
│ QQuery 10 │ 110.24ms │ 105.75ms │ no change │
│ QQuery 11 │ 76.23ms │ 74.64ms │ no change │
│ QQuery 12 │ 60.00ms │ 59.89ms │ no change │
│ QQuery 13 │ 110.87ms │ 113.69ms │ no change │
│ QQuery 14 │ 18.27ms │ 18.63ms │ no change │
│ QQuery 15 │ 31.41ms │ 30.55ms │ no change │
│ QQuery 16 │ 45.93ms │ 45.72ms │ no change │
│ QQuery 17 │ 149.03ms │ 147.66ms │ no change │
│ QQuery 18 │ 498.69ms │ 528.42ms │ 1.06x slower │
│ QQuery 19 │ 61.05ms │ 60.85ms │ no change │
│ QQuery 20 │ 119.30ms │ 107.69ms │ +1.11x faster │
│ QQuery 21 │ 319.51ms │ 319.68ms │ no change │
│ QQuery 22 │ 29.00ms │ 28.43ms │ no change │
└──────────────┴──────────┴──────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (b412dba) │ 2605.35ms │
│ Total Time (dc5cf09) │ 2615.92ms │
│ Average Time (b412dba) │ 118.42ms │
│ Average Time (dc5cf09) │ 118.91ms │
│ Queries Faster │ 1 │
│ Queries Slower │ 2 │
│ Queries with No Change │ 19 │
└────────────────────────┴───────────┘
--------------------
Benchmark tpch_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Query ┃ b412dba ┃ dc5cf09 ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━┩
│ QQuery 1 │ 428.55ms │ 429.60ms │ no change │
│ QQuery 2 │ 56.11ms │ 56.12ms │ no change │
│ QQuery 3 │ 143.48ms │ 141.43ms │ no change │
│ QQuery 4 │ 86.21ms │ 86.00ms │ no change │
│ QQuery 5 │ 200.36ms │ 196.78ms │ no change │
│ QQuery 6 │ 106.66ms │ 105.24ms │ no change │
│ QQuery 7 │ 270.62ms │ 289.56ms │ 1.07x slower │
│ QQuery 8 │ 187.43ms │ 184.49ms │ no change │
│ QQuery 9 │ 285.36ms │ 289.37ms │ no change │
│ QQuery 10 │ 225.58ms │ 230.45ms │ no change │
│ QQuery 11 │ 42.72ms │ 41.21ms │ no change │
│ QQuery 12 │ 127.18ms │ 127.13ms │ no change │
│ QQuery 13 │ 176.86ms │ 176.04ms │ no change │
│ QQuery 14 │ 124.67ms │ 124.03ms │ no change │
│ QQuery 15 │ 188.81ms │ 183.89ms │ no change │
│ QQuery 16 │ 50.40ms │ 48.29ms │ no change │
│ QQuery 17 │ 298.97ms │ 302.80ms │ no change │
│ QQuery 18 │ 432.58ms │ 450.10ms │ no change │
│ QQuery 19 │ 226.26ms │ 226.74ms │ no change │
│ QQuery 20 │ 187.81ms │ 184.51ms │ no change │
│ QQuery 21 │ 315.67ms │ 314.31ms │ no change │
│ QQuery 22 │ 40.97ms │ 39.12ms │ no change │
└──────────────┴──────────┴──────────┴──────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (b412dba) │ 4203.25ms │
│ Total Time (dc5cf09) │ 4227.21ms │
│ Average Time (b412dba) │ 191.06ms │
│ Average Time (dc5cf09) │ 192.15ms │
│ Queries Faster │ 0 │
│ Queries Slower │ 1 │
│ Queries with No Change │ 21 │
└────────────────────────┴───────────┘
--------------------
Benchmark tpch_sf10.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Query ┃ b412dba ┃ dc5cf09 ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━┩
│ QQuery 1 │ 4203.72ms │ 4191.48ms │ no change │
│ QQuery 2 │ 487.60ms │ 507.13ms │ no change │
│ QQuery 3 │ 1702.79ms │ 1687.60ms │ no change │
│ QQuery 4 │ 803.77ms │ 818.32ms │ no change │
│ QQuery 5 │ 2192.67ms │ 2146.59ms │ no change │
│ QQuery 6 │ 1038.22ms │ 1008.99ms │ no change │
│ QQuery 7 │ 3504.91ms │ 3500.76ms │ no change │
│ QQuery 8 │ 2448.41ms │ 2428.53ms │ no change │
│ QQuery 9 │ 3999.62ms │ 3898.60ms │ no change │
│ QQuery 10 │ 2486.28ms │ 2513.15ms │ no change │
│ QQuery 11 │ 349.37ms │ 342.49ms │ no change │
│ QQuery 12 │ 1198.27ms │ 1216.85ms │ no change │
│ QQuery 13 │ 2292.78ms │ 2315.45ms │ no change │
│ QQuery 14 │ 1266.50ms │ 1267.54ms │ no change │
│ QQuery 15 │ 1948.21ms │ 1910.24ms │ no change │
│ QQuery 16 │ 496.71ms │ 509.15ms │ no change │
│ QQuery 17 │ 5150.71ms │ 5092.54ms │ no change │
│ QQuery 18 │ 6569.11ms │ 6690.56ms │ no change │
│ QQuery 19 │ 2256.56ms │ 2230.59ms │ no change │
│ QQuery 20 │ 2539.69ms │ 2512.55ms │ no change │
│ QQuery 21 │ 4329.77ms │ 4335.05ms │ no change │
│ QQuery 22 │ 464.62ms │ 475.21ms │ no change │
└──────────────┴───────────┴───────────┴───────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (b412dba) │ 51730.28ms │
│ Total Time (dc5cf09) │ 51599.36ms │
│ Average Time (b412dba) │ 2351.38ms │
│ Average Time (dc5cf09) │ 2345.43ms │
│ Queries Faster │ 0 │
│ Queries Slower │ 0 │
│ Queries with No Change │ 22 │
└────────────────────────┴────────────┘
```
</details>
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1595307866
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -656,24 +656,16 @@ enum VisitRecord {
EnterMark(usize),
/// the node's children were skipped => jump to f_up on same node
JumpMark,
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
/// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
/// before it.
- fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
+ fn pop_enter_mark(&mut self) -> Option<usize> {
Review Comment:
> I am still a little unclear on if the bug you described above is something that can actually be hit in practice (due to lack of a test case), or if it will be masked by #10413
That's a good question. I think once #10413 gets resolved we can hit the identifier collision issue due to this PR. But actually, let's try to add an identifier collision test case after #10413.
I would suggest 1., revert the commit and a new version of the PR where only the aliases are made simpler.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1595248066
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -885,15 +876,15 @@ mod test {
)?;
let expected = vec![
- (8, "{(SUM(a + Int32(1)) - AVG(c)) * Int32(2)|{Int32(2)}|{SUM(a + Int32(1)) - AVG(c)|{AVG(c)|{c}}|{SUM(a + Int32(1))|{a + Int32(1)|{Int32(1)}|{a}}}}}"),
- (6, "{SUM(a + Int32(1)) - AVG(c)|{AVG(c)|{c}}|{SUM(a + Int32(1))|{a + Int32(1)|{Int32(1)}|{a}}}}"),
+ (8, "#{(SUM(a + Int32(1)) - AVG(c)) * Int32(2)}"),
Review Comment:
`IdArray` shouldn't change by this PR. Actually, new fields can be added to the tuples but the identifier field should remain as it is.
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -885,15 +876,15 @@ mod test {
)?;
let expected = vec![
- (8, "{(SUM(a + Int32(1)) - AVG(c)) * Int32(2)|{Int32(2)}|{SUM(a + Int32(1)) - AVG(c)|{AVG(c)|{c}}|{SUM(a + Int32(1))|{a + Int32(1)|{Int32(1)}|{a}}}}}"),
- (6, "{SUM(a + Int32(1)) - AVG(c)|{AVG(c)|{c}}|{SUM(a + Int32(1))|{a + Int32(1)|{Int32(1)}|{a}}}}"),
+ (8, "#{(SUM(a + Int32(1)) - AVG(c)) * Int32(2)}"),
Review Comment:
`IdArray` shouldn't change in this PR. Actually, new fields can be added to the tuples but the identifier field should remain as it is.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#issuecomment-2102411178
At this point I think we'd better revert this, close it and revisit it after #10413. @alamb @peter-toth
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#issuecomment-2101056433
Per the discussion on https://github.com/apache/datafusion/pull/10396, we merged that one first and now this one needs to be rebased / resolved. Marking it as draft until we can do that
I believe @MohamedAbdeen21 plans to do it this weekend (per https://github.com/apache/datafusion/pull/10396#issuecomment-2099130452)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10333:
URL: https://github.com/apache/datafusion/pull/10333
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1595307866
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -656,24 +656,16 @@ enum VisitRecord {
EnterMark(usize),
/// the node's children were skipped => jump to f_up on same node
JumpMark,
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
/// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
/// before it.
- fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
+ fn pop_enter_mark(&mut self) -> Option<usize> {
Review Comment:
> I am still a little unclear on if the bug you described above is something that can actually be hit in practice (due to lack of a test case), or if it will be masked by #10413
That's a good question. I think once #10413 gets resolved we can hit the identifier collision issue. Actually, let's try to add an identifier collision test case after #10413.
I would suggest 1. and a new version of this PR where only the aliases are made simpler.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#issuecomment-2096549010
Thanks for the review @alamb.
I was trying to implement it duckdb style with `#0`, `#1`, etc ... But as I'm not super familiar with the optimizer, I failed to do so, maybe someone can help.
The biggest problem I found was that the rule is applied twice, meaning that we need a shared/global mapping of expr -> id between rule calls.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#issuecomment-2102581042
Sounds like the consensus is to revert this PR -- could you possible make a revert PR @MohamedAbdeen21 ?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1595307866
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -656,24 +656,16 @@ enum VisitRecord {
EnterMark(usize),
/// the node's children were skipped => jump to f_up on same node
JumpMark,
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
/// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
/// before it.
- fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
+ fn pop_enter_mark(&mut self) -> Option<usize> {
Review Comment:
> I am still a little unclear on if the bug you described above is something that can actually be hit in practice (due to lack of a test case), or if it will be masked by #10413
That's a good question. I think once #10413 gets resolved we can hit the identifier collision issue. But actually, let's try to add an identifier collision test case after #10413.
I would suggest 1., revert the commit and a new version of the PR where only the aliases are made simpler.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1595242837
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -656,24 +656,16 @@ enum VisitRecord {
EnterMark(usize),
/// the node's children were skipped => jump to f_up on same node
JumpMark,
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
/// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
/// before it.
- fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
+ fn pop_enter_mark(&mut self) -> Option<usize> {
Review Comment:
We shoudn't change this part.
The logic that builds up an identifier using `visit_stack` / 3 kinds of `VisitRecord` is neccessary and actually a very clever and way to build up an identifier from the current node and sub-identifiers. (An identifier to be a `String` was not that a clever decision and will be fixed in https://github.com/apache/datafusion/issues/10426, but that's a different issue).
This PR shouldn't change what an identifier is / how it is built up otherwise we end up with identifier colliding bugs again. The `IdArray`, `ExprStats` and `CommonExprs` datastructures require an identifier to represent a full expression subtreee. This means that:
```
fn expr_identifier(expr: &Expr) -> Identifier {
format!("#{{{expr}}}")
}
```
would cause bugs as shown in 1. of https://github.com/apache/datafusion/pull/10396.
I.e. if we encountered both `col("a") + col("b")` and `col("a + b")` in the expression list to be CSEd and we used `"{expr}"` (the non-unique stringified representation) as identifiers then the equal identifier (`"a + b"`) of those 2 different expressions would collide and we counted 2 for the occurance of one of the 2 expressions (and the other expression's count would be lost) resulting wrong CSE.
Please note that currently the identifier of `col("a") + col("b")` is `"{a + b|b|a}"` so it doesn't collide with `col("a + b")`'s identifier: `"{a + b}"`.
Again, this is hard to test now because of the resolution bug: https://github.com/apache/datafusion/issues/10413.
I.e. if we wrote a test where we have
```
select a + b, "a + b" from (
select 1 as a, 2 as b, 1 as "a + b"
)
```
then currently it gets resolved as
```
select "a + b", "a + b" from (
select 1 as a, 2 as b, 1 as "a + b"
)
```
and this prevents me to create a test case for CSE identifier collision.
(Please note that I'm simplifying the identifier collision exmple as simple columns (`col("a + b")`) are not subject to CSE.)
What this PR can do is to change the aliases (use something else than identifiers) to make the plans more readable.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "MohamedAbdeen21 (via GitHub)" <gi...@apache.org>.
MohamedAbdeen21 commented on PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#issuecomment-2094890040
I don't think removing the accumulation and a couple of clones will increase perf, but let's see
/benchmark
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1591315724
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -592,109 +591,51 @@ impl ExprMask {
/// Go through an expression tree and generate identifiers for each subexpression.
///
-/// An identifier contains information of the expression itself and its sub-expression.
-/// This visitor implementation use a stack `visit_stack` to track traversal, which
-/// lets us know when a sub-tree's visiting is finished. When `pre_visit` is called
-/// (traversing to a new node), an `EnterMark` and an `ExprItem` will be pushed into stack.
-/// And try to pop out a `EnterMark` on leaving a node (`f_up()`). All `ExprItem`
-/// before the first `EnterMark` is considered to be sub-tree of the leaving node.
-///
-/// This visitor also records identifier in `id_array`. Makes the following traverse
-/// pass can get the identifier of a node without recalculate it. We assign each node
-/// in the expr tree a series number, start from 1, maintained by `series_number`.
-/// Series number represents the order we left (`f_up()`) a node. Has the property
-/// that child node's series number always smaller than parent's. While `id_array` is
-/// organized in the order we enter (`f_down()`) a node. `node_count` helps us to
-/// get the index of `id_array` for each node.
+/// This visitor also assigns an alias symbol to each expression, to be used if the
+/// expression is getting eliminated, and tracks the frequency and datatype of each expression.
///
/// `Expr` without sub-expr (column, literal etc.) will not have identifier
/// because they should not be recognized as common sub-expr.
struct ExprIdentifierVisitor<'a> {
- // param
+ // expr set to be populated by the visitor
expr_set: &'a mut ExprSet,
/// input schema for the node that we're optimizing, so we can determine the correct datatype
/// for each subexpression
input_schema: DFSchemaRef,
- // inner states
- visit_stack: Vec<VisitRecord>,
- /// increased in fn_down, start from 0.
- node_count: usize,
/// which expression should be skipped?
expr_mask: ExprMask,
}
-/// Record item that used when traversing a expression tree.
-enum VisitRecord {
- /// `usize` is the monotone increasing series number assigned in pre_visit().
- /// Starts from 0. Is used to index the identifier array `id_array` in post_visit().
- EnterMark(usize),
- /// the node's children were skipped => jump to f_up on same node
- JumpMark(usize),
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
-}
-
-impl ExprIdentifierVisitor<'_> {
- /// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
- /// before it.
- fn pop_enter_mark(&mut self) -> (usize, Identifier) {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
- match item {
- VisitRecord::EnterMark(idx) | VisitRecord::JumpMark(idx) => {
- return (idx, desc);
- }
- VisitRecord::ExprItem(id) => {
- desc.push_str(&id);
- }
- }
- }
- unreachable!("Enter mark should paired with node number");
- }
-}
-
impl TreeNodeVisitor for ExprIdentifierVisitor<'_> {
type Node = Expr;
fn f_down(&mut self, expr: &Expr) -> Result<TreeNodeRecursion> {
// related to https://github.com/apache/datafusion/issues/8814
// If the expr contain volatile expression or is a short-circuit expression, skip it.
if expr.short_circuits() || expr.is_volatile()? {
- self.visit_stack
- .push(VisitRecord::JumpMark(self.node_count));
return Ok(TreeNodeRecursion::Jump); // go to f_up
}
- self.visit_stack
- .push(VisitRecord::EnterMark(self.node_count));
- self.node_count += 1;
-
Ok(TreeNodeRecursion::Continue)
}
fn f_up(&mut self, expr: &Expr) -> Result<TreeNodeRecursion> {
- let (_idx, sub_expr_identifier) = self.pop_enter_mark();
-
// skip exprs should not be recognize.
if self.expr_mask.ignores(expr) {
- let curr_expr_identifier = ExprSet::expr_identifier(expr);
- self.visit_stack
- .push(VisitRecord::ExprItem(curr_expr_identifier));
return Ok(TreeNodeRecursion::Continue);
}
- let curr_expr_identifier = ExprSet::expr_identifier(expr);
- let alias_symbol = format!("{curr_expr_identifier}{sub_expr_identifier}");
- self.visit_stack
- .push(VisitRecord::ExprItem(alias_symbol.clone()));
+ let curr_expr_identifier = ExprSet::expr_identifier(expr);
let data_type = expr.get_type(&self.input_schema)?;
+ let alias_symbol = format!("#{{{curr_expr_identifier}}}");
Review Comment:
A small nit might be to pull this into a function that can be documented (so the code that creates identifiers is easier to find)
Perhaps something like
```rust
let alias_symbol = ExprSet::alias_symbol(expr)
```
🤔
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1595307866
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -656,24 +656,16 @@ enum VisitRecord {
EnterMark(usize),
/// the node's children were skipped => jump to f_up on same node
JumpMark,
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
/// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
/// before it.
- fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
+ fn pop_enter_mark(&mut self) -> Option<usize> {
Review Comment:
> I am still a little unclear on if the bug you described above is something that can actually be hit in practice (due to lack of a test case), or if it will be masked by #10413
That's a good question. I think once #10413 gets resolved we can hit the identifier collision issue. But actually, let's try to add an identifier collision test case after #10413.
I would suggest 1. and a new version of this PR where only the aliases are made simpler.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1595242837
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -656,24 +656,16 @@ enum VisitRecord {
EnterMark(usize),
/// the node's children were skipped => jump to f_up on same node
JumpMark,
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
/// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
/// before it.
- fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
+ fn pop_enter_mark(&mut self) -> Option<usize> {
Review Comment:
We shoudn't change this part.
The logic that builds up an identifier using `visit_stack` / 3 kinds of `VisitRecord` is neccessary and actually a very clever and way to build up an identifier from the current node and sub-identifiers. (An identifier to be a `String` was not that a clever decision and will be fixed in https://github.com/apache/datafusion/issues/10426, but that's a different issue).
This PR shouldn't change what an identifier is / how it is built up otherwise we end up with identifier colliding bugs again. The `IdArray`, `ExprStats` and `CommonExprs` datastructures require an dentifier to represent a full expression subtreee. This means that:
```
fn expr_identifier(expr: &Expr) -> Identifier {
format!("#{{{expr}}}")
}
```
would cause bugs as shown in 1. of https://github.com/apache/datafusion/pull/10396.
I.e. if we encountered both `col("a") + col("b")` and `col("a + b")` in the expression list to be CSEd and we used `"{expr}"` (the non-unique stringified representation) as identifiers then the equal identifier (`"a + b"`) of those 2 different expressions would collide and we counted 2 for the occurance of one of the 2 expressions (and the other expression's count would be lost) resulting wrong CSE.
Please note that currently the identifier of `col("a") + col("b")` is `"{a + b|b|a}"` so it doesn't collide with `col("a + b")`'s identifier: `"{a + b}"`.
Again, this is hard to test now because of the resolution bug: https://github.com/apache/datafusion/issues/10413.
I.e. if we wrote a test where we have
```
select a + b, "a + b" from (
select 1 as a, 2 as b, 1 as "a + b"
)
```
then currently it gets resolved as
```
select "a + b", "a + b" from (
select 1 as a, 2 as b, 1 as "a + b"
)
```
and this prevents me to create a test case for CSE identifier collision.
(Please note that I'm simplifying the identifier collision exmple as simple columns (`col("a + b")`) are not subject to CSE.)
What this PR can do is to change the aliases (use something else than identifiers) to make the plans more readable.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1595278451
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -656,24 +656,16 @@ enum VisitRecord {
EnterMark(usize),
/// the node's children were skipped => jump to f_up on same node
JumpMark,
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
/// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
/// before it.
- fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
+ fn pop_enter_mark(&mut self) -> Option<usize> {
Review Comment:
@peter-toth -- thank you -- I see now the danger of the lack of test coverage in this area.
I am still a little unclear on if the bug you described above is something that can actually be hit in practice (due to lack of a test case), or if it will be masked by https://github.com/apache/datafusion/issues/10413
What shall we do now? I can see three possibilities:
1. Revert this PR and try and make a follow on one that doesn't change it
2. Create a PR to revert just ID change
3. You can fix the ID change in your fix for https://github.com/apache/datafusion/issues/10426
Please let me know your thoughts. I can potentially help with 1 or 2.
cc @MohamedAbdeen21
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] make common expression alias human-readable [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10333:
URL: https://github.com/apache/datafusion/pull/10333#discussion_r1596787866
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -656,24 +656,16 @@ enum VisitRecord {
EnterMark(usize),
/// the node's children were skipped => jump to f_up on same node
JumpMark,
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
/// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
/// before it.
- fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
- let mut desc = String::new();
-
- while let Some(item) = self.visit_stack.pop() {
+ fn pop_enter_mark(&mut self) -> Option<usize> {
Review Comment:
Revert PR is merged: https://github.com/apache/datafusion/pull/10436
And I am pretty stoked that @jonahgao is looking at https://github.com/apache/datafusion/issues/10413#issuecomment-2104080364
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org