You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "PhVHoang (via GitHub)" <gi...@apache.org> on 2024/02/12 14:29:57 UTC
[PR] Return an error when a column does not exist in window function [arrow-datafusion]
PhVHoang opened a new pull request, #9202:
URL: https://github.com/apache/arrow-datafusion/pull/9202
## Which issue does this PR close?
Closes #9166.
## Rationale for this change
To return an error instead of panicking when columns don't exist in window function.
## What changes are included in this PR?
Code, tests
## Are these changes tested?
Yes
## Are there any user-facing changes?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return an error when a column does not exist in window function [arrow-datafusion]
Posted by "PhVHoang (via GitHub)" <gi...@apache.org>.
PhVHoang commented on PR #9202:
URL: https://github.com/apache/arrow-datafusion/pull/9202#issuecomment-1944194486
@alamb @comphead I added some more tests. Please take a look and let me know if these tests are sufficient for this change.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return an error when a column does not exist in window function [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9202:
URL: https://github.com/apache/arrow-datafusion/pull/9202
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return an error when a column does not exist in window function [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9202:
URL: https://github.com/apache/arrow-datafusion/pull/9202#issuecomment-1943898220
> @PhVHoang please add the test also for non existent partition by column and qualified column like `t.a`, `schema.t.a`, `catalog.schema.t.a`
I think the only thing remaining before merging this PR is the suggestion from @comphead https://github.com/apache/arrow-datafusion/pull/9202#issuecomment-1939321659 for a few more tests. I can add them hopefully later today if no one else has a chance
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return an error when a column does not exist in window function [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #9202:
URL: https://github.com/apache/arrow-datafusion/pull/9202#issuecomment-1939321659
@PhVHoang please add the test also for non existent partition by column
and qualified column like `t.a`, `schema.t.a`, `catalog.schema.t.a`
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return an error when a column does not exist in window function [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9202:
URL: https://github.com/apache/arrow-datafusion/pull/9202#issuecomment-1944817114
Thanks again everyone
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return an error when a column does not exist in window function [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9202:
URL: https://github.com/apache/arrow-datafusion/pull/9202#discussion_r1486296187
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -149,18 +149,37 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
)?;
let func_deps = schema.functional_dependencies();
- // Find whether ties are possible in the given ordering:
- let is_ordering_strict = order_by.iter().any(|orderby_expr| {
+ // Find whether ties are possible in the given ordering
+ let is_ordering_strict = order_by.iter().find_map(|orderby_expr| {
if let Expr::Sort(sort_expr) = orderby_expr {
if let Expr::Column(col) = sort_expr.expr.as_ref() {
- let idx = schema.index_of_column(col).unwrap();
- return func_deps.iter().any(|dep| {
+ let idx_result = schema.index_of_column(col);
+
+ // Propagate the error to the caller and return it immediately
+ let idx = idx_result
Review Comment:
I think the `ok()` function call here discards the error.
I think changing
```rust
let idx = schema.index_of_column(col).unwrap();
```
To
```rust
let idx = schema.index_of_column(col).ok()?;
```
accomplishes the same thing (and your test still passes)
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return an error when a column does not exist in window function [arrow-datafusion]
Posted by "PhVHoang (via GitHub)" <gi...@apache.org>.
PhVHoang commented on code in PR #9202:
URL: https://github.com/apache/arrow-datafusion/pull/9202#discussion_r1486375492
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -149,18 +149,37 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
)?;
let func_deps = schema.functional_dependencies();
- // Find whether ties are possible in the given ordering:
- let is_ordering_strict = order_by.iter().any(|orderby_expr| {
+ // Find whether ties are possible in the given ordering
+ let is_ordering_strict = order_by.iter().find_map(|orderby_expr| {
if let Expr::Sort(sort_expr) = orderby_expr {
if let Expr::Column(col) = sort_expr.expr.as_ref() {
- let idx = schema.index_of_column(col).unwrap();
- return func_deps.iter().any(|dep| {
+ let idx_result = schema.index_of_column(col);
+
+ // Propagate the error to the caller and return it immediately
+ let idx = idx_result
Review Comment:
Thank you very much for your suggestion. Yes it's way simpler than that of mine.
Fixed.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org