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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
