You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/10/31 09:54:53 UTC
[PR] Minor: error on unsupported RESPECT NULLs syntax [arrow-datafusion]
alamb opened a new pull request, #7998:
URL: https://github.com/apache/arrow-datafusion/pull/7998
## Which issue does this PR close?
Follow on to https://github.com/apache/arrow-datafusion/pull/7983 from @jackwener
## Rationale for this change
While reviewing https://github.com/apache/arrow-datafusion/pull/7983 I noticed there was a newly introduced field in SQLParser's `Function` struct that is ignored by DataFusion
## What changes are included in this PR?
1. If the unsupported syntax is included, return a not yet implemented error
## Are these changes tested?
Yes, with a new test
## Are there any user-facing changes?
Will not silently accept (and ignore) unsupported syntax
<!--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: error on unsupported RESPECT NULLs syntax [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #7998:
URL: https://github.com/apache/arrow-datafusion/pull/7998#discussion_r1377849063
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -1287,6 +1287,17 @@ fn select_simple_aggregate_repeated_aggregate_with_unique_aliases() {
);
}
+#[test]
+fn select_simple_aggregate_respect_nulls() {
+ let sql = "SELECT MIN(age) RESPECT NULLS FROM person";
+ let err = logical_plan(sql).expect_err("query should have failed");
+
+ // HashSet doesn't guarantee order
Review Comment:
```suggestion
```
--
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] Minor: error on unsupported RESPECT NULLs syntax [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7998:
URL: https://github.com/apache/arrow-datafusion/pull/7998#discussion_r1377328247
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -36,44 +36,57 @@ use super::arrow_cast::ARROW_CAST_NAME;
impl<'a, S: ContextProvider> SqlToRel<'a, S> {
pub(super) fn sql_function_to_expr(
&self,
- mut function: SQLFunction,
+ function: SQLFunction,
schema: &DFSchema,
planner_context: &mut PlannerContext,
) -> Result<Expr> {
- let name = if function.name.0.len() > 1 {
+ let SQLFunction {
Review Comment:
I also changed to use this explicit destructuring, so if other new fields are added the compiler will tell us in the future
--
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] Minor: error on unsupported RESPECT NULLs syntax [arrow-datafusion]
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on code in PR #7998:
URL: https://github.com/apache/arrow-datafusion/pull/7998#discussion_r1377343249
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -36,44 +36,57 @@ use super::arrow_cast::ARROW_CAST_NAME;
impl<'a, S: ContextProvider> SqlToRel<'a, S> {
pub(super) fn sql_function_to_expr(
&self,
- mut function: SQLFunction,
+ function: SQLFunction,
schema: &DFSchema,
planner_context: &mut PlannerContext,
) -> Result<Expr> {
- let name = if function.name.0.len() > 1 {
+ let SQLFunction {
Review Comment:
👍Good idea
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -36,44 +36,57 @@ use super::arrow_cast::ARROW_CAST_NAME;
impl<'a, S: ContextProvider> SqlToRel<'a, S> {
pub(super) fn sql_function_to_expr(
&self,
- mut function: SQLFunction,
+ function: SQLFunction,
schema: &DFSchema,
planner_context: &mut PlannerContext,
) -> Result<Expr> {
- let name = if function.name.0.len() > 1 {
+ let SQLFunction {
Review Comment:
👍Good idea
--
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] Minor: error on unsupported RESPECT NULLs syntax [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7998:
URL: https://github.com/apache/arrow-datafusion/pull/7998
--
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