You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "notfilippo (via GitHub)" <gi...@apache.org> on 2024/10/22 12:16:12 UTC
[PR] feat: improve type inference for WindowFrame [datafusion]
notfilippo opened a new pull request, #13059:
URL: https://github.com/apache/datafusion/pull/13059
## Which issue does this PR close?
Closes #11432
## 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.
-->
This tried to fix the liked issue by improving the type inference when creating window frame from sqlparser.
## 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.
-->
- WindowFrameBound's try_from method is replaced by a try_parse that also takes the WindowFrame's units in order to make some conclusions about the type.
## 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)?
-->
Yes
## 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.
-->
No API changes but potentially a new behaviour introduced that could break some existing code.
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1814844142
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
+ v: ast::Expr,
+ units: &ast::WindowFrameUnits,
+) -> Result<ScalarValue> {
+ match units {
+ // For ROWS and GROUPS we are sure that the ScalarValue must be a non-negative integer ...
+ ast::WindowFrameUnits::Rows | ast::WindowFrameUnits::Groups => match v {
+ ast::Expr::Value(ast::Value::Number(value, false))
+ | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => {
Review Comment:
This means a single quoted string will always be parsed as an interval 🤷
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1812223451
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -1061,7 +1061,7 @@ fn test_aggregation_to_sql() {
FROM person
GROUP BY id, first_name;"#,
r#"SELECT person.id, person.first_name,
-sum(person.id) AS total_sum, sum(person.id) OVER (PARTITION BY person.first_name ROWS BETWEEN '5' PRECEDING AND '2' FOLLOWING) AS moving_sum,
+sum(person.id) AS total_sum, sum(person.id) OVER (PARTITION BY person.first_name ROWS BETWEEN 5 PRECEDING AND 2 FOLLOWING) AS moving_sum,
Review Comment:
Aha! Even more fixes!
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1814765591
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
+ v: ast::Expr,
+ units: &ast::WindowFrameUnits,
+) -> Result<ScalarValue> {
+ match units {
+ // For ROWS and GROUPS we are sure that the ScalarValue must be a non-negative integer ...
+ ast::WindowFrameUnits::Rows | ast::WindowFrameUnits::Groups => match v {
+ ast::Expr::Value(ast::Value::Number(value, false))
+ | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => {
Review Comment:
Curiously, do you know what SQL (or substrait?) will generate this case, Rows with SingleQuotedString? I'm wondering if we should keep this matching rule or remove it.
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1810628066
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -479,8 +498,123 @@ mod tests {
ast::Expr::Value(ast::Value::Number("1".to_string(), false)),
)))),
};
- let result = WindowFrame::try_from(window_frame);
- assert!(result.is_ok());
+
+ let window_frame = WindowFrame::try_from(window_frame)?;
+ assert_eq!(window_frame.units, WindowFrameUnits::Rows);
+ assert_eq!(
+ window_frame.start_bound,
+ WindowFrameBound::Preceding(ScalarValue::UInt64(Some(2)))
+ );
+ assert_eq!(
+ window_frame.end_bound,
+ WindowFrameBound::Preceding(ScalarValue::UInt64(Some(1)))
+ );
+
+ Ok(())
+ }
+
+ macro_rules! test_bound {
+ ($unit:ident, $value:expr, $expected:expr) => {
+ let preceding = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Preceding($value),
+ &ast::WindowFrameUnits::$unit,
+ )?;
+ assert_eq!(preceding, WindowFrameBound::Preceding($expected));
+ let following = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Following($value),
+ &ast::WindowFrameUnits::$unit,
+ )?;
+ assert_eq!(following, WindowFrameBound::Following($expected));
+ };
+ }
+
+ macro_rules! test_bound_err {
+ ($unit:ident, $value:expr, $expected:expr) => {
+ let err = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Preceding($value),
+ &ast::WindowFrameUnits::$unit,
+ )
+ .unwrap_err();
+ assert_eq!(err.strip_backtrace(), $expected);
+ let err = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Following($value),
+ &ast::WindowFrameUnits::$unit,
+ )
+ .unwrap_err();
+ assert_eq!(err.strip_backtrace(), $expected);
+ };
+ }
+
+ #[test]
+ fn test_window_frame_bound_creation() -> Result<()> {
+ // Unbounded
+ test_bound!(Rows, None, ScalarValue::Null);
+ test_bound!(Groups, None, ScalarValue::Null);
+ test_bound!(Range, None, ScalarValue::Null);
+
+ // Numeric (Number)
+ let number = Some(Box::new(ast::Expr::Value(ast::Value::Number(
+ "42".to_string(),
+ false,
+ ))));
+ test_bound!(Rows, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(Groups, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(
+ Range,
+ number.clone(),
+ ScalarValue::Utf8(Some("42".to_string()))
+ );
+
+ // Numeric (SingleQuotedString)
+ let number = Some(Box::new(ast::Expr::Value(ast::Value::SingleQuotedString(
+ "42".to_string(),
+ ))));
+ test_bound!(Rows, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(Groups, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(
+ Range,
+ number.clone(),
+ ScalarValue::Utf8(Some("42".to_string()))
+ );
+
+ // Non-numeric (SingleQuotedString)
+ let non_numeric = Some(Box::new(ast::Expr::Value(
+ ast::Value::SingleQuotedString("false".to_string()),
+ )));
+ test_bound_err!(
+ Rows,
+ non_numeric.clone(),
+ "Arrow error: Cast error: Cannot cast string 'false' to value of UInt64 type"
+ );
+ test_bound_err!(
+ Groups,
+ non_numeric.clone(),
+ "Arrow error: Cast error: Cannot cast string 'false' to value of UInt64 type"
+ );
+ test_bound!(
+ Range,
+ non_numeric.clone(),
+ ScalarValue::Utf8(Some("false".to_string()))
+ );
Review Comment:
Not really sure if this is expected but essentially I've tried to keep Range with the original behaviour, only changing Rows and Groups since we know they are certainly UInt64
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1814842636
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
+ v: ast::Expr,
+ units: &ast::WindowFrameUnits,
+) -> Result<ScalarValue> {
+ match units {
+ // For ROWS and GROUPS we are sure that the ScalarValue must be a non-negative integer ...
+ ast::WindowFrameUnits::Rows | ast::WindowFrameUnits::Groups => match v {
+ ast::Expr::Value(ast::Value::Number(value, false))
+ | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => {
Review Comment:
The relevant parsing code is here: https://github.com/apache/datafusion-sqlparser-rs/blob/8e0d26abb3c4dda2fa091dfd9611be1dbdc14755/src/parser/mod.rs#L1483-L1490
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1812524550
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -479,8 +498,123 @@ mod tests {
ast::Expr::Value(ast::Value::Number("1".to_string(), false)),
)))),
};
- let result = WindowFrame::try_from(window_frame);
- assert!(result.is_ok());
+
+ let window_frame = WindowFrame::try_from(window_frame)?;
+ assert_eq!(window_frame.units, WindowFrameUnits::Rows);
+ assert_eq!(
+ window_frame.start_bound,
+ WindowFrameBound::Preceding(ScalarValue::UInt64(Some(2)))
+ );
+ assert_eq!(
+ window_frame.end_bound,
+ WindowFrameBound::Preceding(ScalarValue::UInt64(Some(1)))
+ );
+
+ Ok(())
+ }
+
+ macro_rules! test_bound {
+ ($unit:ident, $value:expr, $expected:expr) => {
+ let preceding = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Preceding($value),
+ &ast::WindowFrameUnits::$unit,
+ )?;
+ assert_eq!(preceding, WindowFrameBound::Preceding($expected));
+ let following = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Following($value),
+ &ast::WindowFrameUnits::$unit,
+ )?;
+ assert_eq!(following, WindowFrameBound::Following($expected));
+ };
+ }
+
+ macro_rules! test_bound_err {
+ ($unit:ident, $value:expr, $expected:expr) => {
+ let err = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Preceding($value),
+ &ast::WindowFrameUnits::$unit,
+ )
+ .unwrap_err();
+ assert_eq!(err.strip_backtrace(), $expected);
+ let err = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Following($value),
+ &ast::WindowFrameUnits::$unit,
+ )
+ .unwrap_err();
+ assert_eq!(err.strip_backtrace(), $expected);
+ };
+ }
+
+ #[test]
+ fn test_window_frame_bound_creation() -> Result<()> {
+ // Unbounded
+ test_bound!(Rows, None, ScalarValue::Null);
+ test_bound!(Groups, None, ScalarValue::Null);
+ test_bound!(Range, None, ScalarValue::Null);
+
+ // Numeric (Number)
+ let number = Some(Box::new(ast::Expr::Value(ast::Value::Number(
+ "42".to_string(),
+ false,
+ ))));
+ test_bound!(Rows, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(Groups, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(
+ Range,
+ number.clone(),
+ ScalarValue::Utf8(Some("42".to_string()))
+ );
+
+ // Numeric (SingleQuotedString)
+ let number = Some(Box::new(ast::Expr::Value(ast::Value::SingleQuotedString(
+ "42".to_string(),
+ ))));
+ test_bound!(Rows, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(Groups, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(
+ Range,
+ number.clone(),
+ ScalarValue::Utf8(Some("42".to_string()))
+ );
+
+ // Non-numeric (SingleQuotedString)
+ let non_numeric = Some(Box::new(ast::Expr::Value(
+ ast::Value::SingleQuotedString("false".to_string()),
+ )));
+ test_bound_err!(
+ Rows,
+ non_numeric.clone(),
+ "Arrow error: Cast error: Cannot cast string 'false' to value of UInt64 type"
+ );
+ test_bound_err!(
+ Groups,
+ non_numeric.clone(),
+ "Arrow error: Cast error: Cannot cast string 'false' to value of UInt64 type"
+ );
+ test_bound!(
+ Range,
+ non_numeric.clone(),
+ ScalarValue::Utf8(Some("false".to_string()))
+ );
Review Comment:
I guess this is alright as it is existing behaviour.
When I tried a query with `RANGE INTERVAL 'false' PRECEDING` in `datafusion-cli` (main branch) it failed like this:
```sh
type_coercion
caused by
Arrow error: Parser error: Invalid input syntax for type interval: "false"
```
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1810675746
##########
datafusion/sqllogictest/test_files/window.slt:
##########
@@ -2208,7 +2208,7 @@ physical_plan
01)ProjectionExec: expr=[sum1@0 as sum1, sum2@1 as sum2]
02)--SortExec: TopK(fetch=5), expr=[c9@2 ASC NULLS LAST], preserve_partitioning=[false]
03)----ProjectionExec: expr=[sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST, aggregate_test_100.c2 ASC NULLS LAST] GROUPS BETWEEN 1 PRECEDING AND 1 FOLLOWING@3 as sum1, sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST] GROUPS BETWEEN 5 PRECEDING AND 3 PRECEDING@4 as sum2, c9@1 as c9]
-04)------BoundedWindowAggExec: wdw=[sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST] GROUPS BETWEEN 5 PRECEDING AND 3 PRECEDING: Ok(Field { name: "sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST] GROUPS BETWEEN 5 PRECEDING AND 3 PRECEDING", data_type: Float64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Groups, start_bound: Preceding(UInt64(5)), end_bound: Preceding(UInt64(3)), is_causal: false }], mode=[Sorted]
+04)------BoundedWindowAggExec: wdw=[sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST] GROUPS BETWEEN 5 PRECEDING AND 3 PRECEDING: Ok(Field { name: "sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST] GROUPS BETWEEN 5 PRECEDING AND 3 PRECEDING", data_type: Float64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Groups, start_bound: Preceding(UInt64(5)), end_bound: Preceding(UInt64(3)), is_causal: true }], mode=[Sorted]
Review Comment:
Looks like this exposed a bug in existing test 👍.
The `is_causal: true` in plan output, looks correct (because the start & end of the window frame is in the past).
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1814827972
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
+ v: ast::Expr,
+ units: &ast::WindowFrameUnits,
+) -> Result<ScalarValue> {
+ match units {
+ // For ROWS and GROUPS we are sure that the ScalarValue must be a non-negative integer ...
+ ast::WindowFrameUnits::Rows | ast::WindowFrameUnits::Groups => match v {
+ ast::Expr::Value(ast::Value::Number(value, false))
+ | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => {
Review Comment:
Curiously I'm not sure it will show up. It will either be Interval or Value(Number). So I guess we can remove it.
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#issuecomment-2435303973
Thanks @notfilippo
If no more comments from the other one, I plan to merge this tomorrow.
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1810773165
##########
datafusion/sqllogictest/test_files/window.slt:
##########
@@ -2208,7 +2208,7 @@ physical_plan
01)ProjectionExec: expr=[sum1@0 as sum1, sum2@1 as sum2]
02)--SortExec: TopK(fetch=5), expr=[c9@2 ASC NULLS LAST], preserve_partitioning=[false]
03)----ProjectionExec: expr=[sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST, aggregate_test_100.c2 ASC NULLS LAST] GROUPS BETWEEN 1 PRECEDING AND 1 FOLLOWING@3 as sum1, sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST] GROUPS BETWEEN 5 PRECEDING AND 3 PRECEDING@4 as sum2, c9@1 as c9]
-04)------BoundedWindowAggExec: wdw=[sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST] GROUPS BETWEEN 5 PRECEDING AND 3 PRECEDING: Ok(Field { name: "sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST] GROUPS BETWEEN 5 PRECEDING AND 3 PRECEDING", data_type: Float64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Groups, start_bound: Preceding(UInt64(5)), end_bound: Preceding(UInt64(3)), is_causal: false }], mode=[Sorted]
+04)------BoundedWindowAggExec: wdw=[sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST] GROUPS BETWEEN 5 PRECEDING AND 3 PRECEDING: Ok(Field { name: "sum(aggregate_test_100.c12) ORDER BY [aggregate_test_100.c1 ASC NULLS LAST] GROUPS BETWEEN 5 PRECEDING AND 3 PRECEDING", data_type: Float64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Groups, start_bound: Preceding(UInt64(5)), end_bound: Preceding(UInt64(3)), is_causal: true }], mode=[Sorted]
Review Comment:
Yes! It's very nice to have a bit more correctness when parsing from SQL.
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1813049155
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
Review Comment:
As a public method, how about adding some doc for it?
##########
datafusion/sqllogictest/test_files/window.slt:
##########
@@ -2378,15 +2378,15 @@ SELECT c9, rn1 FROM (SELECT c9,
# invalid window frame. null as preceding
-statement error DataFusion error: Error during planning: Invalid window frame: frame offsets must be non negative integers
+statement error DataFusion error: Error during planning: Invalid window frame: frame offsets for ROWS / GROUPS must be non negative integers
select row_number() over (rows between null preceding and current row) from (select 1 a) x
Review Comment:
I didn't find the negative test for the `GROUPS` case. It would be nice if we added it.
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -1730,98 +1730,38 @@ fn make_substrait_like_expr(
}
}
+fn to_substrait_bound_offset(value: &ScalarValue) -> Option<i64> {
Review Comment:
nice refactor. 👍
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
+ v: ast::Expr,
+ units: &ast::WindowFrameUnits,
+) -> Result<ScalarValue> {
+ match units {
+ // For ROWS and GROUPS we are sure that the ScalarValue must be a non-negative integer ...
+ ast::WindowFrameUnits::Rows | ast::WindowFrameUnits::Groups => match v {
+ ast::Expr::Value(ast::Value::Number(value, false))
+ | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => {
Review Comment:
I tried to add some sqllogictests case for `SinglQuotedString` but I found it doesn't work in end-to-end SQL test.
```
External error: statement failed: DataFusion error: Error during planning: Invalid window frame: frame offsets for ROWS / GROUPS must be non negative integers
[SQL] select row_number() over (rows between '1' preceding and current row) from (select 1 a) x
at test_files/window.slt:2381
```
I think this worked in the previous version but it isn't covered in the tests. 🤔
```
DataFusion CLI v42.0.0
> select row_number() over (rows between '1' preceding and current row) from (select 1 a) x;
+-------------------------------------------------------+
| row_number() ROWS BETWEEN 1 PRECEDING AND CURRENT ROW |
+-------------------------------------------------------+
| 1 |
+-------------------------------------------------------+
1 row(s) fetched.
Elapsed 0.001 seconds.
```
I think we should add some `SingleQuotedString` case in the sqllogictests.
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1813399305
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
Review Comment:
Better yet it should not be public
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1812524550
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -479,8 +498,123 @@ mod tests {
ast::Expr::Value(ast::Value::Number("1".to_string(), false)),
)))),
};
- let result = WindowFrame::try_from(window_frame);
- assert!(result.is_ok());
+
+ let window_frame = WindowFrame::try_from(window_frame)?;
+ assert_eq!(window_frame.units, WindowFrameUnits::Rows);
+ assert_eq!(
+ window_frame.start_bound,
+ WindowFrameBound::Preceding(ScalarValue::UInt64(Some(2)))
+ );
+ assert_eq!(
+ window_frame.end_bound,
+ WindowFrameBound::Preceding(ScalarValue::UInt64(Some(1)))
+ );
+
+ Ok(())
+ }
+
+ macro_rules! test_bound {
+ ($unit:ident, $value:expr, $expected:expr) => {
+ let preceding = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Preceding($value),
+ &ast::WindowFrameUnits::$unit,
+ )?;
+ assert_eq!(preceding, WindowFrameBound::Preceding($expected));
+ let following = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Following($value),
+ &ast::WindowFrameUnits::$unit,
+ )?;
+ assert_eq!(following, WindowFrameBound::Following($expected));
+ };
+ }
+
+ macro_rules! test_bound_err {
+ ($unit:ident, $value:expr, $expected:expr) => {
+ let err = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Preceding($value),
+ &ast::WindowFrameUnits::$unit,
+ )
+ .unwrap_err();
+ assert_eq!(err.strip_backtrace(), $expected);
+ let err = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Following($value),
+ &ast::WindowFrameUnits::$unit,
+ )
+ .unwrap_err();
+ assert_eq!(err.strip_backtrace(), $expected);
+ };
+ }
+
+ #[test]
+ fn test_window_frame_bound_creation() -> Result<()> {
+ // Unbounded
+ test_bound!(Rows, None, ScalarValue::Null);
+ test_bound!(Groups, None, ScalarValue::Null);
+ test_bound!(Range, None, ScalarValue::Null);
+
+ // Numeric (Number)
+ let number = Some(Box::new(ast::Expr::Value(ast::Value::Number(
+ "42".to_string(),
+ false,
+ ))));
+ test_bound!(Rows, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(Groups, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(
+ Range,
+ number.clone(),
+ ScalarValue::Utf8(Some("42".to_string()))
+ );
+
+ // Numeric (SingleQuotedString)
+ let number = Some(Box::new(ast::Expr::Value(ast::Value::SingleQuotedString(
+ "42".to_string(),
+ ))));
+ test_bound!(Rows, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(Groups, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(
+ Range,
+ number.clone(),
+ ScalarValue::Utf8(Some("42".to_string()))
+ );
+
+ // Non-numeric (SingleQuotedString)
+ let non_numeric = Some(Box::new(ast::Expr::Value(
+ ast::Value::SingleQuotedString("false".to_string()),
+ )));
+ test_bound_err!(
+ Rows,
+ non_numeric.clone(),
+ "Arrow error: Cast error: Cannot cast string 'false' to value of UInt64 type"
+ );
+ test_bound_err!(
+ Groups,
+ non_numeric.clone(),
+ "Arrow error: Cast error: Cannot cast string 'false' to value of UInt64 type"
+ );
+ test_bound!(
+ Range,
+ non_numeric.clone(),
+ ScalarValue::Utf8(Some("false".to_string()))
+ );
Review Comment:
I guess this is alright as it is existing behaviour.
When I tried a query with `RANGE INTERVAL 'false' PRECEDING` in `datafusion-cli` it failed like this:
```sh
type_coercion
caused by
Arrow error: Parser error: Invalid input syntax for type interval: "false"
```
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1813237745
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
+ v: ast::Expr,
+ units: &ast::WindowFrameUnits,
+) -> Result<ScalarValue> {
+ match units {
+ // For ROWS and GROUPS we are sure that the ScalarValue must be a non-negative integer ...
+ ast::WindowFrameUnits::Rows | ast::WindowFrameUnits::Groups => match v {
+ ast::Expr::Value(ast::Value::Number(value, false))
+ | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => {
Review Comment:
Great catch!
The `rows between '1' preceding and current row` maps to the following AST expression.
```
Interval(
Interval {
value: Value(
SingleQuotedString(
"1",
),
),
leading_field: None,
leading_precision: None,
last_field: None,
fractional_seconds_precision: None,
},
)
```
It seems like for both `Rows` and `Groups` we need to also handle the `ast::Expr::Interval(...)` pattern to preserve existing behaviour.
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#issuecomment-2436983493
Thanks again @notfilippo @jcsherin 👍
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal merged PR #13059:
URL: https://github.com/apache/datafusion/pull/13059
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#issuecomment-2435147352
@goldmedal I've removed SingleQuotedString according to https://github.com/apache/datafusion/pull/13059#discussion_r1814842636.
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1812217865
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -1061,7 +1061,7 @@ fn test_aggregation_to_sql() {
FROM person
GROUP BY id, first_name;"#,
r#"SELECT person.id, person.first_name,
-sum(person.id) AS total_sum, sum(person.id) OVER (PARTITION BY person.first_name ROWS BETWEEN '5' PRECEDING AND '2' FOLLOWING) AS moving_sum,
+sum(person.id) AS total_sum, sum(person.id) OVER (PARTITION BY person.first_name ROWS BETWEEN 5 PRECEDING AND 2 FOLLOWING) AS moving_sum,
Review Comment:
- #12982
FYI @notfilippo
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1812524550
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -479,8 +498,123 @@ mod tests {
ast::Expr::Value(ast::Value::Number("1".to_string(), false)),
)))),
};
- let result = WindowFrame::try_from(window_frame);
- assert!(result.is_ok());
+
+ let window_frame = WindowFrame::try_from(window_frame)?;
+ assert_eq!(window_frame.units, WindowFrameUnits::Rows);
+ assert_eq!(
+ window_frame.start_bound,
+ WindowFrameBound::Preceding(ScalarValue::UInt64(Some(2)))
+ );
+ assert_eq!(
+ window_frame.end_bound,
+ WindowFrameBound::Preceding(ScalarValue::UInt64(Some(1)))
+ );
+
+ Ok(())
+ }
+
+ macro_rules! test_bound {
+ ($unit:ident, $value:expr, $expected:expr) => {
+ let preceding = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Preceding($value),
+ &ast::WindowFrameUnits::$unit,
+ )?;
+ assert_eq!(preceding, WindowFrameBound::Preceding($expected));
+ let following = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Following($value),
+ &ast::WindowFrameUnits::$unit,
+ )?;
+ assert_eq!(following, WindowFrameBound::Following($expected));
+ };
+ }
+
+ macro_rules! test_bound_err {
+ ($unit:ident, $value:expr, $expected:expr) => {
+ let err = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Preceding($value),
+ &ast::WindowFrameUnits::$unit,
+ )
+ .unwrap_err();
+ assert_eq!(err.strip_backtrace(), $expected);
+ let err = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Following($value),
+ &ast::WindowFrameUnits::$unit,
+ )
+ .unwrap_err();
+ assert_eq!(err.strip_backtrace(), $expected);
+ };
+ }
+
+ #[test]
+ fn test_window_frame_bound_creation() -> Result<()> {
+ // Unbounded
+ test_bound!(Rows, None, ScalarValue::Null);
+ test_bound!(Groups, None, ScalarValue::Null);
+ test_bound!(Range, None, ScalarValue::Null);
+
+ // Numeric (Number)
+ let number = Some(Box::new(ast::Expr::Value(ast::Value::Number(
+ "42".to_string(),
+ false,
+ ))));
+ test_bound!(Rows, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(Groups, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(
+ Range,
+ number.clone(),
+ ScalarValue::Utf8(Some("42".to_string()))
+ );
+
+ // Numeric (SingleQuotedString)
+ let number = Some(Box::new(ast::Expr::Value(ast::Value::SingleQuotedString(
+ "42".to_string(),
+ ))));
+ test_bound!(Rows, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(Groups, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(
+ Range,
+ number.clone(),
+ ScalarValue::Utf8(Some("42".to_string()))
+ );
+
+ // Non-numeric (SingleQuotedString)
+ let non_numeric = Some(Box::new(ast::Expr::Value(
+ ast::Value::SingleQuotedString("false".to_string()),
+ )));
+ test_bound_err!(
+ Rows,
+ non_numeric.clone(),
+ "Arrow error: Cast error: Cannot cast string 'false' to value of UInt64 type"
+ );
+ test_bound_err!(
+ Groups,
+ non_numeric.clone(),
+ "Arrow error: Cast error: Cannot cast string 'false' to value of UInt64 type"
+ );
+ test_bound!(
+ Range,
+ non_numeric.clone(),
+ ScalarValue::Utf8(Some("false".to_string()))
+ );
Review Comment:
I guess this is alright as it does not change any existing behaviour. The query will still fail to parse.
When I checked with `RANGE INTERVAL 'false' PRECEDING` in `datafusion-cli` (main branch) it failed like this:
```sh
type_coercion
caused by
Arrow error: Parser error: Invalid input syntax for type interval: "false"
```
This same error is reproduced in this branch as well.
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -479,8 +498,123 @@ mod tests {
ast::Expr::Value(ast::Value::Number("1".to_string(), false)),
)))),
};
- let result = WindowFrame::try_from(window_frame);
- assert!(result.is_ok());
+
+ let window_frame = WindowFrame::try_from(window_frame)?;
+ assert_eq!(window_frame.units, WindowFrameUnits::Rows);
+ assert_eq!(
+ window_frame.start_bound,
+ WindowFrameBound::Preceding(ScalarValue::UInt64(Some(2)))
+ );
+ assert_eq!(
+ window_frame.end_bound,
+ WindowFrameBound::Preceding(ScalarValue::UInt64(Some(1)))
+ );
+
+ Ok(())
+ }
+
+ macro_rules! test_bound {
+ ($unit:ident, $value:expr, $expected:expr) => {
+ let preceding = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Preceding($value),
+ &ast::WindowFrameUnits::$unit,
+ )?;
+ assert_eq!(preceding, WindowFrameBound::Preceding($expected));
+ let following = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Following($value),
+ &ast::WindowFrameUnits::$unit,
+ )?;
+ assert_eq!(following, WindowFrameBound::Following($expected));
+ };
+ }
+
+ macro_rules! test_bound_err {
+ ($unit:ident, $value:expr, $expected:expr) => {
+ let err = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Preceding($value),
+ &ast::WindowFrameUnits::$unit,
+ )
+ .unwrap_err();
+ assert_eq!(err.strip_backtrace(), $expected);
+ let err = WindowFrameBound::try_parse(
+ ast::WindowFrameBound::Following($value),
+ &ast::WindowFrameUnits::$unit,
+ )
+ .unwrap_err();
+ assert_eq!(err.strip_backtrace(), $expected);
+ };
+ }
+
+ #[test]
+ fn test_window_frame_bound_creation() -> Result<()> {
+ // Unbounded
+ test_bound!(Rows, None, ScalarValue::Null);
+ test_bound!(Groups, None, ScalarValue::Null);
+ test_bound!(Range, None, ScalarValue::Null);
+
+ // Numeric (Number)
+ let number = Some(Box::new(ast::Expr::Value(ast::Value::Number(
+ "42".to_string(),
+ false,
+ ))));
+ test_bound!(Rows, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(Groups, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(
+ Range,
+ number.clone(),
+ ScalarValue::Utf8(Some("42".to_string()))
+ );
+
+ // Numeric (SingleQuotedString)
+ let number = Some(Box::new(ast::Expr::Value(ast::Value::SingleQuotedString(
+ "42".to_string(),
+ ))));
+ test_bound!(Rows, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(Groups, number.clone(), ScalarValue::UInt64(Some(42)));
+ test_bound!(
+ Range,
+ number.clone(),
+ ScalarValue::Utf8(Some("42".to_string()))
+ );
+
+ // Non-numeric (SingleQuotedString)
+ let non_numeric = Some(Box::new(ast::Expr::Value(
+ ast::Value::SingleQuotedString("false".to_string()),
+ )));
+ test_bound_err!(
+ Rows,
+ non_numeric.clone(),
+ "Arrow error: Cast error: Cannot cast string 'false' to value of UInt64 type"
+ );
+ test_bound_err!(
+ Groups,
+ non_numeric.clone(),
+ "Arrow error: Cast error: Cannot cast string 'false' to value of UInt64 type"
+ );
+ test_bound!(
+ Range,
+ non_numeric.clone(),
+ ScalarValue::Utf8(Some("false".to_string()))
+ );
Review Comment:
I guess this is alright as it does not change any existing behaviour. The query will still fail to parse.
When I checked with `RANGE INTERVAL 'false' PRECEDING` in `datafusion-cli` (main branch) it failed like this:
```sh
type_coercion
caused by
Arrow error: Parser error: Invalid input syntax for type interval: "false"
```
The same error is reproduced in this branch as well.
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1813399305
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
Review Comment:
Better yet it should not be public
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1813426471
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
Review Comment:
Better yet it should not be public
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#issuecomment-2431855033
The substrait plan in the [original reproducer](https://github.com/apache/datafusion/issues/11432#issue-2405650302) now contains the expected window frame values for `ROWS BETWEEN 1 PRECEDING AND 1 FOLLOWING`:
```
bounds_type: Rows,
lower_bound: Some(
Bound {
kind: Some(
Preceding(
Preceding {
offset: 1,
}
),
),
},
),
upper_bound: Some(
Bound {
kind: Some(
Following(
Following {
offset: 1,
}
),
),
},
),
```
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#discussion_r1813237745
##########
datafusion/expr/src/window_frame.rs:
##########
@@ -334,51 +335,69 @@ impl WindowFrameBound {
}
}
-impl TryFrom<ast::WindowFrameBound> for WindowFrameBound {
- type Error = DataFusionError;
-
- fn try_from(value: ast::WindowFrameBound) -> Result<Self> {
+impl WindowFrameBound {
+ fn try_parse(
+ value: ast::WindowFrameBound,
+ units: &ast::WindowFrameUnits,
+ ) -> Result<Self> {
Ok(match value {
ast::WindowFrameBound::Preceding(Some(v)) => {
- Self::Preceding(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Preceding(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Preceding(None) => Self::Preceding(ScalarValue::Null),
ast::WindowFrameBound::Following(Some(v)) => {
- Self::Following(convert_frame_bound_to_scalar_value(*v)?)
+ Self::Following(convert_frame_bound_to_scalar_value(*v, units)?)
}
ast::WindowFrameBound::Following(None) => Self::Following(ScalarValue::Null),
ast::WindowFrameBound::CurrentRow => Self::CurrentRow,
})
}
}
-pub fn convert_frame_bound_to_scalar_value(v: ast::Expr) -> Result<ScalarValue> {
- Ok(ScalarValue::Utf8(Some(match v {
- ast::Expr::Value(ast::Value::Number(value, false))
- | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => value,
- ast::Expr::Interval(ast::Interval {
- value,
- leading_field,
- ..
- }) => {
- let result = match *value {
- ast::Expr::Value(ast::Value::SingleQuotedString(item)) => item,
- e => {
- return sql_err!(ParserError(format!(
- "INTERVAL expression cannot be {e:?}"
- )));
+pub fn convert_frame_bound_to_scalar_value(
+ v: ast::Expr,
+ units: &ast::WindowFrameUnits,
+) -> Result<ScalarValue> {
+ match units {
+ // For ROWS and GROUPS we are sure that the ScalarValue must be a non-negative integer ...
+ ast::WindowFrameUnits::Rows | ast::WindowFrameUnits::Groups => match v {
+ ast::Expr::Value(ast::Value::Number(value, false))
+ | ast::Expr::Value(ast::Value::SingleQuotedString(value)) => {
Review Comment:
Great catch!
The `rows between '1' preceding and current row` maps to the following AST expression.
```
Interval(
Interval {
value: Value(
SingleQuotedString(
"1",
),
),
leading_field: None,
leading_precision: None,
last_field: None,
fractional_seconds_precision: None,
},
)
```
It seems like for `Groups` we need to also handle the `ast::Expr::Interval(...)` pattern to preserve existing behaviour.
--
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] feat: improve type inference for WindowFrame [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on PR #13059:
URL: https://github.com/apache/datafusion/pull/13059#issuecomment-2431863674
@notfilippo Thanks, squashed three bugs with this refactor 🚀.
cc @alamb
--
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