You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "chenkovsky (via GitHub)" <gi...@apache.org> on 2025/04/06 10:54:59 UTC
[PR] fix: union all by name [datafusion]
chenkovsky opened a new pull request, #15603:
URL: https://github.com/apache/datafusion/pull/15603
## Which issue does this PR close?
- Closes #15394.
## Rationale for this change
schema from inner physical plan is returned.
## What changes are included in this PR?
update UnionExec and RecordBatchStreamAdapter to transform schema.
I found that logical plan's nullability for Projection is also not correct after optimization,
But this won't make the test fail. So I haven't included this part in this PR. Do we need to correct logical plan?
## Are these changes tested?
UT
## Are there any user-facing changes?
No
--
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] fix: union all by name [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #15603:
URL: https://github.com/apache/datafusion/pull/15603#discussion_r2033557454
##########
datafusion/physical-plan/src/stream.rs:
##########
@@ -408,7 +422,18 @@ where
type Item = Result<RecordBatch>;
fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
- self.project().stream.poll_next(cx)
+ let this = self.project();
+ let transform_schema = *this.transform_schema;
+ let schema = Arc::clone(this.schema);
+ let ret = this.stream.poll_next(cx);
+ if transform_schema {
+ if let Poll::Ready(Some(Ok(batch))) = ret {
+ return Poll::Ready(Some(batch.with_schema(schema).map_err(|e| {
Review Comment:
I think this is one of the notorious problems when logical schema doesn't match the physical one on nullability/metadata. But this change might bring a performance impact, although the schema change is just reassigning the value but it also calls `schema_contains` which may be expensive
--
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] fix: union all by name [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #15603:
URL: https://github.com/apache/datafusion/pull/15603#discussion_r2030153718
##########
datafusion/physical-plan/src/stream.rs:
##########
@@ -362,6 +362,8 @@ pin_project! {
#[pin]
stream: S,
+
+ transform_schema: bool,
Review Comment:
this seems like this is fixing the symptom rather than the root cause
I think it would be better to have the correct schema reflected in the plan in the first place 🤔
--
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] fix: union all by name [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on code in PR #15603:
URL: https://github.com/apache/datafusion/pull/15603#discussion_r2033572970
##########
datafusion/physical-plan/src/stream.rs:
##########
@@ -408,7 +422,18 @@ where
type Item = Result<RecordBatch>;
fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
- self.project().stream.poll_next(cx)
+ let this = self.project();
+ let transform_schema = *this.transform_schema;
+ let schema = Arc::clone(this.schema);
+ let ret = this.stream.poll_next(cx);
+ if transform_schema {
+ if let Poll::Ready(Some(Ok(batch))) = ret {
+ return Poll::Ready(Some(batch.with_schema(schema).map_err(|e| {
Review Comment:
yes, there's performance concern now. if this approach is feasible, I can try to optimize it, maybe use RecordBatch::try_new_with_options
--
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] fix: union all by name [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on code in PR #15603:
URL: https://github.com/apache/datafusion/pull/15603#discussion_r2030172787
##########
datafusion/physical-plan/src/stream.rs:
##########
@@ -362,6 +362,8 @@ pin_project! {
#[pin]
stream: S,
+
+ transform_schema: bool,
Review Comment:
yes, correct nullability in schema is better. I tried to fix logical plan before.
But nullability in logical plan won't affect physical plan. it's ignored.
https://github.com/apache/datafusion/blob/dccf3778e95037f3ed8740627799e9d658943157/datafusion/core/src/physical_planner.rs#L763
in physical plan, it will recompute nullaibility from bottom to top.
https://github.com/apache/datafusion/blob/dccf3778e95037f3ed8740627799e9d658943157/datafusion/physical-plan/src/projection.rs#L85
but in this scenario, it seems that we need to pass nullability from top to bottom.
I need more suggestions.
--
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] fix: union all by name [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15603:
URL: https://github.com/apache/datafusion/pull/15603#issuecomment-2961018203
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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] fix: union all by name [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #15603:
URL: https://github.com/apache/datafusion/pull/15603#issuecomment-2790862097
Thanks for looking into the nullable issue, it's been on my plate for a bit to look into some more. It's really the last blocker I know of for union by name to work correctly.
--
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] fix: union all by name [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on code in PR #15603:
URL: https://github.com/apache/datafusion/pull/15603#discussion_r2030547375
##########
datafusion/physical-plan/src/stream.rs:
##########
@@ -362,6 +362,8 @@ pin_project! {
#[pin]
stream: S,
+
+ transform_schema: bool,
Review Comment:
I want to learn some experience from spark.
for logical plan, I haven't found any logic to handle this problem.
https://github.com/apache/spark/blob/75d80c7795ca71d24229010ab04ae740473126aa/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala#L475
for physical plan, spark is much easier, its InternalRow is schemaless. so it will use the schema of physical plan by default. but recordbatch contains schema.
https://github.com/apache/spark/blob/75d80c7795ca71d24229010ab04ae740473126aa/sql/core/src/main/scala/org/apache/spark/sql/execution/basicPhysicalOperators.scala#L688
I'm not 100% sure, I think current logical plan and physical plan schema is correct. the root cause is that recordbatch's schema doesn't match physical plan's. so adding an adapter is a proper way.
--
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] fix: union all by name [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #15603: fix: union all by name
URL: https://github.com/apache/datafusion/pull/15603
--
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