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: [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]
