alamb commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614057653
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) ->
alamb commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2127444050
I hope to review this PR later today or tomorrow
--
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
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611618653
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
If we can assume th
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611618653
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
If we can assume th
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611436607
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
Ah ok, I got it now
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611436607
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
Ah ok, I got it now
berkaysynnada commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611413134
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
In a world where
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611302863
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
That's true, but pl
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611302863
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
That's true, but pl
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611302863
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
That's true, but pl
berkaysynnada commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611257842
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
`with_new_childr
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611241009
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
We need to be able
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611241009
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
We need to restore
berkaysynnada commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611219929
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
Can we skip push
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1611205100
##
datafusion/physical-expr/src/expressions/case.rs:
##
@@ -258,6 +259,10 @@ impl CaseExpr {
}
}
+lazy_static! {
Review Comment:
We can't create tem
berkaysynnada commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2126385035
Thanks a lot, @peter-toth! This looks great to me. The new `children(&self)
-> Vec<&Arc`> signature also uncovers implicit clones, and
many of them seem to be redundant. Our Tr
peter-toth commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2125641924
I've rebased the PR on the latest `main`. The first commit is exactly the
same as the previous state of this PR was, the second commit changes
`TreeNode:apply()` and `TreeNode:vis
peter-toth commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2122829132
> What do we think about merging this PR and filing a follow on ticket to
unify the APIs?
I'm ok with merging the current state of the PR. But I was also thinking
about how
alamb commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2121191619
What do we think about merging this PR and filing a follow on ticket to
unify the APIs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please
ozankabak commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2120338004
No worries. Will be happy to review and help iterate once you are ready
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to Gi
peter-toth commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2120329963
I'm still working on an alternative to this PR and will need a couple of
more days to test a few different ideas...
--
This is an automated message from the Apache Git Service.
peter-toth commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2117088548
@ozankabak, let me check the changes required for the alternative today or
tomorrow and come back to you.
--
This is an automated message from the Apache Git Service.
To respond
ozankabak commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2116059936
Breaking this into two PRs (with the latter one removing the old usage and
migrating to the new one) sounds reasonable to me. It could make sense to do
the follow-on quickly becaus
alamb commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2115952198
I agree with @berkaysynnada in
https://github.com/apache/datafusion/pull/10543#issuecomment-2115389289 that in
an ideal world we woul change `TreeNode::visit` and `TreeNode::apply`, ho
peter-toth commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2115422692
> Thanks @peter-toth. After a quick look, I started thinking it might be
better to use this new ref_visitor API instead of keeping also the original
one. I'll take a closer look t
berkaysynnada commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2115389289
> cc @alamb, @berkaysynnada, @ozankabak
Thanks @peter-toth. After a quick look, I started thinking it might be
better to use this new ref_visitor API instead of keeping a
peter-toth commented on PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2115281194
cc @alamb, @berkaysynnada, @ozankabak
--
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
peter-toth opened a new pull request, #10543:
URL: https://github.com/apache/datafusion/pull/10543
## Which issue does this PR close?
Part of https://github.com/apache/datafusion/issues/10505, required for
https://github.com/apache/datafusion/issues/10426.
## Rationale for thi
28 matches
Mail list logo
