alamb commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3054126649
Thanks again @bert-beyondloops
--
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 spec
alamb merged PR #16632:
URL: https://github.com/apache/datafusion/pull/16632
--
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...@datafusi
alamb commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3050201621
> branch has been rebased. Should I squash commit or is this done during
merge ?
commits are squashed on merge so no need to do it on the branch
Pushing commits rather tha
bert-beyondloops commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3050159701
branch has been rebased
--
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 s
alamb commented on code in PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#discussion_r2192561119
##
datafusion/proto/src/logical_plan/mod.rs:
##
@@ -916,40 +915,13 @@ impl AsLogicalPlan for LogicalPlanNode {
LogicalPlanType::Unnest(unnest) => {
alamb commented on code in PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#discussion_r2192559391
##
datafusion/expr/src/logical_plan/plan.rs:
##
@@ -3995,6 +3995,211 @@ impl PartialOrd for Unnest {
}
}
+impl Unnest {
+pub fn try_new(
Review Comment:
alamb commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3049000941
This PR appears to have a conflict now
https://github.com/user-attachments/assets/4191732d-6c83-4955-b8a4-cbf6304e681a";
/>
--
This is an automated message from the Apache
bert-beyondloops commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3048052607
I moved the logic around the unnest column detection into a new
Unnest::try_new method.
In my opinion, this did not belong into the LogicalPlanBuilder.
The projecti
bert-beyondloops commented on code in PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#discussion_r2191593559
##
datafusion/sqllogictest/test_files/push_down_filter.slt:
##
@@ -39,9 +39,9 @@ physical_plan
01)ProjectionExec: expr=[__unnest_placeholder(v.column2,de
alamb commented on code in PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#discussion_r2191065570
##
datafusion/sqllogictest/test_files/push_down_filter.slt:
##
@@ -39,9 +39,9 @@ physical_plan
01)ProjectionExec: expr=[__unnest_placeholder(v.column2,depth=1)@0 as
alamb commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3044933393
> There are already some tests verifying the explain plans for the unnest
plan. The scheduled run will currently fail.
I'll adapt the 2 test failures
Indeed the exising tests
bert-beyondloops commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3044765606
Hi @alamb,
Thanks for the review.
There are already some tests verifying the explain plans for the unnest
plan. The scheduled run will currently fail.
I'll a
alamb commented on PR #16632:
URL: https://github.com/apache/datafusion/pull/16632#issuecomment-3044638489
Thanks @bert-beyondloops -- sorry for the delay in review
I started the CI tests
The changes in this PR make sense to me -- the only thing I think it needs
is some test t
13 matches
Mail list logo
