You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "epsio-banay (via GitHub)" <gi...@apache.org> on 2024/10/21 16:14:52 UTC
[PR] Fix DISTINCT ON expressions match ORDER BY expressions check - normal… [datafusion]
epsio-banay opened a new pull request, #13039:
URL: https://github.com/apache/datafusion/pull/13039
…ize columns before checking if expressions match
## Which issue does this PR close?
Closes #13038.
## 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?
Yes, unit 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?
No
<!--
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] Fix DISTINCT ON expressions match ORDER BY expressions check - normal… [datafusion]
Posted by "epsio-banay (via GitHub)" <gi...@apache.org>.
epsio-banay commented on code in PR #13039:
URL: https://github.com/apache/datafusion/pull/13039#discussion_r1810602090
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2889,7 +2890,9 @@ impl DistinctOn {
// Check that the left-most sort expressions are the same as the `ON` expressions.
let mut matched = true;
for (on, sort) in self.on_expr.iter().zip(sort_expr.iter()) {
- if on != &sort.expr {
+ if normalize_col(on.clone(), &self.input)?
Review Comment:
Sorry for wasting your time on these tests and thank you for looking into it.
Apparently the bug I experienced was just a little bit different - there is a bug in column normalization where we don't normalize field qualifiers correctly.
for the query "SELECT DISTINCT ON (t.a) t.a AS alert_id FROM public.t ORDER BY t.a"
the sort expression was normalized to public.t.a but the distinct on expression was normalized to t.a.
I added a fix for that in the `normalize_with_schemas_and_ambiguity_check` function.
But I think this PR will now probably effect most of the logical plans that datafusion will generate.
--
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 DISTINCT ON expressions match ORDER BY expressions check - normal… [datafusion]
Posted by "epsio-banay (via GitHub)" <gi...@apache.org>.
epsio-banay commented on code in PR #13039:
URL: https://github.com/apache/datafusion/pull/13039#discussion_r1810602090
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2889,7 +2890,9 @@ impl DistinctOn {
// Check that the left-most sort expressions are the same as the `ON` expressions.
let mut matched = true;
for (on, sort) in self.on_expr.iter().zip(sort_expr.iter()) {
- if on != &sort.expr {
+ if normalize_col(on.clone(), &self.input)?
Review Comment:
Sorry for wasting your time on these tests and thank you for looking into it.
Apparently the bug I experienced was just a little bit different - there is a bug in column normalization where we don't normalize field qualifiers.
for the query "SELECT DISTINCT ON (t.a) t.a AS alert_id FROM public.t ORDER BY t.a"
the sort expression was normalized to public.t.a but the distinct on expression was normalized to t.a.
I added a fix for that in the `normalize_with_schemas_and_ambiguity_check` function.
But I think this PR will now probably effect most of the logical plans that datafusion will generate.
--
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 DISTINCT ON expressions match ORDER BY expressions check - normal… [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #13039: Fix DISTINCT ON expressions match ORDER BY expressions check - normal…
URL: https://github.com/apache/datafusion/pull/13039
--
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 DISTINCT ON expressions match ORDER BY expressions check - normal… [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13039:
URL: https://github.com/apache/datafusion/pull/13039#issuecomment-2558299054
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 DISTINCT ON expressions match ORDER BY expressions check - normal… [datafusion]
Posted by "epsio-banay (via GitHub)" <gi...@apache.org>.
epsio-banay commented on code in PR #13039:
URL: https://github.com/apache/datafusion/pull/13039#discussion_r1810604241
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -3870,6 +3873,39 @@ digraph {
);
}
+ #[test]
+ fn distinct_on_expr_order_by_mismatch() -> Result<()> {
+ let schema = Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ ]);
+
+ let table_scan = table_scan(TableReference::none(), &schema, None)?.build()?;
+ let p = DistinctOn::try_new(
+ vec![col("a")],
+ vec![],
+ Some(vec![SortExpr::new(col("b"), true, false)]),
+ Arc::new(table_scan),
+ );
+ assert_eq!(p.err().unwrap().strip_backtrace(), "Error during planning: SELECT DISTINCT ON expressions must match initial ORDER BY expressions");
+ Ok(())
+ }
+
+ #[test]
+ fn distinct_on_expr_order_by_match() -> Result<()> {
Review Comment:
I changed the tests.
marking as resolved
--
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 DISTINCT ON expressions match ORDER BY expressions check - normal… [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13039:
URL: https://github.com/apache/datafusion/pull/13039#discussion_r1809481258
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2889,7 +2890,9 @@ impl DistinctOn {
// Check that the left-most sort expressions are the same as the `ON` expressions.
let mut matched = true;
for (on, sort) in self.on_expr.iter().zip(sort_expr.iter()) {
- if on != &sort.expr {
+ if normalize_col(on.clone(), &self.input)?
Review Comment:
It seems like the `sort` is already normalized; https://github.com/apache/datafusion/blob/2e274bfbdfc19f11b2951456bb2a48e88733c9bf/datafusion/expr/src/expr_rewriter/mod.rs#L127-L128
Likewise isn't `self.on_exprs` is also already normalized ?
https://github.com/apache/datafusion/blob/c3a919a9b87646c531f96e39e7217c11480b3118/datafusion/expr/src/logical_plan/plan.rs#L2858-L2859
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -3870,6 +3873,39 @@ digraph {
);
}
+ #[test]
Review Comment:
Could you also please add a sqllogictest? Perhaps by extending https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/distinct_on.slt ?
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -3870,6 +3873,39 @@ digraph {
);
}
+ #[test]
+ fn distinct_on_expr_order_by_mismatch() -> Result<()> {
+ let schema = Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ ]);
+
+ let table_scan = table_scan(TableReference::none(), &schema, None)?.build()?;
+ let p = DistinctOn::try_new(
+ vec![col("a")],
+ vec![],
+ Some(vec![SortExpr::new(col("b"), true, false)]),
+ Arc::new(table_scan),
+ );
+ assert_eq!(p.err().unwrap().strip_backtrace(), "Error during planning: SELECT DISTINCT ON expressions must match initial ORDER BY expressions");
+ Ok(())
+ }
+
+ #[test]
+ fn distinct_on_expr_order_by_match() -> Result<()> {
Review Comment:
When I revert the code changes in this PR, these tests still pass 🤔 -- so I am not sure this PR solves the issue
```diff
diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs
index 6f4e78ccf..9d7d893be 100644
--- a/datafusion/expr/src/logical_plan/plan.rs
+++ b/datafusion/expr/src/logical_plan/plan.rs
@@ -28,8 +28,7 @@ use super::DdlStatement;
use crate::builder::{change_redundant_column, unnest_with_options};
use crate::expr::{Placeholder, Sort as SortExpr, WindowFunction};
use crate::expr_rewriter::{
- create_col_from_scalar_expr, normalize_col, normalize_cols, normalize_sorts,
- NamePreserver,
+ create_col_from_scalar_expr, normalize_cols, normalize_sorts, NamePreserver,
};
use crate::logical_plan::display::{GraphvizVisitor, IndentVisitor};
use crate::logical_plan::extension::UserDefinedLogicalNode;
@@ -2890,9 +2889,7 @@ impl DistinctOn {
// Check that the left-most sort expressions are the same as the `ON` expressions.
let mut matched = true;
for (on, sort) in self.on_expr.iter().zip(sort_expr.iter()) {
- if normalize_col(on.clone(), &self.input)?
- != normalize_col(sort.expr.clone(), &self.input)?
- {
+ if on != &sort.expr {
matched = false;
break;
}
```
```shell
(venv) andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$ cargo test -p datafusion-expr -- distinct_on
Compiling datafusion-expr v42.1.0 (/Users/andrewlamb/Software/datafusion/datafusion/expr)
warning: unused variable: `p`
--> datafusion/expr/src/logical_plan/plan.rs:3897:13
|
3897 | let p = DistinctOn::try_new(
| ^ help: if this is intentional, prefix it with an underscore: `_p`
|
= note: `#[warn(unused_variables)]` on by default
warning: `datafusion-expr` (lib test) generated 1 warning
Finished `test` profile [unoptimized + debuginfo] target(s) in 2.99s
Running unittests src/lib.rs (target/debug/deps/datafusion_expr-3be1ac638c9d6fb5)
running 2 tests
test logical_plan::plan::tests::distinct_on_expr_order_by_match ... ok
test logical_plan::plan::tests::distinct_on_expr_order_by_mismatch ... ok
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 107 filtered out; finished in 0.00s
Doc-tests datafusion_expr
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 45 filtered out; finished in 0.01s
```
--
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 DISTINCT ON expressions match ORDER BY expressions check - normal… [datafusion]
Posted by "epsio-banay (via GitHub)" <gi...@apache.org>.
epsio-banay commented on code in PR #13039:
URL: https://github.com/apache/datafusion/pull/13039#discussion_r1810592688
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -3870,6 +3873,39 @@ digraph {
);
}
+ #[test]
Review Comment:
added 👍
--
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