Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
waynexia commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1618334298
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
I've tried to return `&[Arc]` and it fails unsurprisingly
-- unless we stores all children in a vector like `Union`:
```rust
fn children(&self) -> &[Arc] {
self.inputs.as_slice()
}
```
Otherwise we cannot return a temporary slice
--
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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
peter-toth commented on PR #10543: URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2133480467 Thanks all for the review! @alamb, those API example PRs look great! -- 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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
alamb commented on PR #10543: URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2133297368 I made these two PRs to add examples of using TreeNode: 1. https://github.com/apache/datafusion/pull/10686 2. https://github.com/apache/datafusion/pull/10687 Also a bonus to improve: https://github.com/apache/datafusion/pull/10685 -- 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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
alamb commented on PR #10543: URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2133166613 Thank you again @peter-toth @ozankabak and @berkaysynnada -- I am woking on some doc examples for this as well -- 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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
alamb merged PR #10543: URL: https://github.com/apache/datafusion/pull/10543 -- 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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
alamb commented on PR #10543: URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2132172812 I filed https://github.com/apache/datafusion/issues/10678 to handle visit_with_subqueries I am waiting to merge this PR until the CI is fixed on main https://github.com/apache/datafusion/pull/10677 -- 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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
alamb commented on PR #10543: URL: https://github.com/apache/datafusion/pull/10543#issuecomment-2131268529 Awesome! I plan to merge this PR tomorrow unless anyone else would like time to review. -- 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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614657172
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
I opened a PR as returning a slice in `ConcreteTreeNode` is possible:
https://github.com/apache/datafusion/pull/10666
But it will not work for `LogicalPlan` or `ExecutionPlan` or `PhysicalExpr`.
--
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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614522462
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
Actualy, I don't think we can return a slice of references. Returning an
empty slice here would be ok, but at other places where there are children to
return (e.g. in `BinaryExpr`) we need to build a temporary container (vec or
array) to store the references of children and then return a slice of the
container, but who will own the container?
--
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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614503459
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
~I think we could return a slice, but the current `Vec` is in sync with
other implementations how we usually return children. Like
`LogicalPlan::inputs()` or `ConcreteTreeNode::children()`.
Or shall we change those too?~
--
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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614522462
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
Actualy, I don't think we can return a slice of references. Returning an
empty slice here would be ok, but at other places where there are children to
return (e.g. in `BinaryExpr`) we need to build a container (vec or array) to
store the references of children and then return a slice of the container, but
who will own the container?
--
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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614522462
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
Actualy, I don't think we can return a slice of references. Returning an
empty slice here would be ok, but at other places where there are children to
return (e.g. in `BinaryExpr`) we need to build container (vec or array) to
store the references and then return a slice of the container, but who will own
the container?
--
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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614522462
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
Actualy, I don't think we can return a slice of references. Returning an
empty slice here would be ok, but at other places where there are childrens to
return (e.g. in `BinaryExpr`) we need to build container (vec or array) to
store the references and then return a slice of the container, but who will own
the container?
--
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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614503459
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
I think we could return a slice, but the current `Vec` is in sync with other
implementations how we usually return children. Like `LogicalPlan::inputs()` or
`ConcreteTreeNode::children()`.
Or shall we change those too?
--
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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614503459
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
I think we could return a slice, but the current `Vec` is in sync with other
implementations how we usually return childrens. Like `LogicalPlan::inputs()`
or `ConcreteTreeNode::children()`.
Or shall we change those too?
--
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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
peter-toth commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614503459
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
I think we could return a slice, but the current `Vec` is in sync with other
implementations how we usually return childrens. Like `LogicalPlan::inputs()`
or `ConcreteTreeNode::children()`.
--
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
Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]
ozankabak commented on code in PR #10543:
URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614484172
##
datafusion/physical-plan/src/work_table.rs:
##
@@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec {
&self.cache
}
-fn children(&self) -> Vec> {
+fn children(&self) -> Vec<&Arc> {
Review Comment:
Maybe `&[&Arc]` was meant?
--
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
