You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Sevenannn (via GitHub)" <gi...@apache.org> on 2024/10/17 03:37:23 UTC
[PR] Don't preserve functional dependency when generating UNION logical plan [datafusion]
Sevenannn opened a new pull request, #12979:
URL: https://github.com/apache/datafusion/pull/12979
* Don't preserve functional dependency when generating UNION logical plan
* Remove extra lines
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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] Don't preserve functional dependency when generating UNION logical plan [datafusion]
Posted by "sgrebnov (via GitHub)" <gi...@apache.org>.
sgrebnov commented on PR #12979:
URL: https://github.com/apache/datafusion/pull/12979#issuecomment-2418596824
LGTM đź‘Ť
--
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] Don't preserve functional dependency when generating UNION logical plan [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #12979:
URL: https://github.com/apache/datafusion/pull/12979
--
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] Don't preserve functional dependency when generating UNION logical plan [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #12979:
URL: https://github.com/apache/datafusion/pull/12979#discussion_r1806062077
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1402,7 +1402,12 @@ pub fn validate_unique_names<'a>(
pub fn union(left_plan: LogicalPlan, right_plan: LogicalPlan) -> Result<LogicalPlan> {
// Temporarily use the schema from the left input and later rely on the analyzer to
// coerce the two schemas into a common one.
- let schema = Arc::clone(left_plan.schema());
+
+ // Functional Dependencies doesn't preserve after UNION operation
+ let schema = (**left_plan.schema()).clone();
+ let schema =
+ Arc::new(schema.with_functional_dependencies(FunctionalDependencies::empty())?);
+
Review Comment:
Is clearing out all dependencies the right fix? Could we retain some if they do not harm?
##########
datafusion/core/src/dataframe/mod.rs:
##########
@@ -2623,6 +2623,54 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn test_aggregate_with_union() -> Result<()> {
+ let df = test_table().await?;
+
+ let df1 = df
+ .clone()
+ // GROUP BY `c1`
+ .aggregate(vec![col("c1")], vec![min(col("c2"))])?
+ // SELECT `c1` , min(c2) as `result`
+ .select(vec![col("c1"), min(col("c2")).alias("result")])?;
+ let df2 = df
+ .clone()
+ // GROUP BY `c1`
+ .aggregate(vec![col("c1")], vec![max(col("c3"))])?
+ // SELECT `c1` , max(c3) as `result`
+ .select(vec![col("c1"), max(col("c3")).alias("result")])?;
+
+ let df_union = df1.union(df2)?;
+ let df = df_union
+ // GROUP BY `c1`
+ .aggregate(
+ vec![col("c1")],
+ vec![sum(col("result")).alias("sum_result")],
+ )?
+ // SELECT `c1`, sum(result) as `sum_result`
+ .select(vec![(col("c1")), col("sum_result")])?;
+
+ let df_results = df.collect().await?;
+
+ #[rustfmt::skip]
+ assert_batches_sorted_eq!(
+ [
+ "+----+------------+",
+ "| c1 | sum_result |",
+ "+----+------------+",
+ "| a | 84 |",
+ "| b | 69 |",
+ "| c | 124 |",
+ "| d | 126 |",
+ "| e | 121 |",
+ "+----+------------+"
+ ],
+ &df_results
+ );
+
+ Ok(())
+ }
+
Review Comment:
I tried this test before this PR, and it fails as @Sevenannn explained. The current result matches what postgre provides
--
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] Don't preserve functional dependency when generating UNION logical plan [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12979:
URL: https://github.com/apache/datafusion/pull/12979#discussion_r1806919102
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1402,7 +1402,12 @@ pub fn validate_unique_names<'a>(
pub fn union(left_plan: LogicalPlan, right_plan: LogicalPlan) -> Result<LogicalPlan> {
// Temporarily use the schema from the left input and later rely on the analyzer to
// coerce the two schemas into a common one.
- let schema = Arc::clone(left_plan.schema());
+
+ // Functional Dependencies doesn't preserve after UNION operation
+ let schema = (**left_plan.schema()).clone();
+ let schema =
+ Arc::new(schema.with_functional_dependencies(FunctionalDependencies::empty())?);
+
Review Comment:
I'll wait to merge this PR until tomorrow to give @Sevenannn a chance to respond
--
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] Don't preserve functional dependency when generating UNION logical plan [datafusion]
Posted by "Sevenannn (via GitHub)" <gi...@apache.org>.
Sevenannn commented on code in PR #12979:
URL: https://github.com/apache/datafusion/pull/12979#discussion_r1807113509
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1402,7 +1402,12 @@ pub fn validate_unique_names<'a>(
pub fn union(left_plan: LogicalPlan, right_plan: LogicalPlan) -> Result<LogicalPlan> {
// Temporarily use the schema from the left input and later rely on the analyzer to
// coerce the two schemas into a common one.
- let schema = Arc::clone(left_plan.schema());
+
+ // Functional Dependencies doesn't preserve after UNION operation
+ let schema = (**left_plan.schema()).clone();
+ let schema =
+ Arc::new(schema.with_functional_dependencies(FunctionalDependencies::empty())?);
+
Review Comment:
Hi @berkaysynnada, thanks for the review! I don’t think any FD persists when performing UNION on 2 tables.
A simple example would be UNION table t1 with another table t2 which only has 1 row, there always exists such data in t2 which could break the FDs in t1 / t2 after the UNION.
In this case, clearing FDs would be the right fix since we don’t want FDs to get wrongly retained and affect later plans, e.g. aggregation.
Please let me know if you have any further questions regarding this PR, thanks!
--
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] Don't preserve functional dependency when generating UNION logical plan [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12979:
URL: https://github.com/apache/datafusion/pull/12979#issuecomment-2424917360
Thanks again @Sevenannn and @sgrebnov and @berkaysynnada
--
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