-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix: handle NULL input in lead/lag window function #12811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -217,6 +217,41 @@ fn get_casted_value( | |
| } | ||
| } | ||
|
|
||
| /// Rewrites the NULL expression (1st argument) with an expression | ||
| /// which is the same data type as the default value (3rd argument). | ||
| /// Also rewrites the return type with the same data type as the | ||
| /// default value. | ||
| /// | ||
| /// If a default value is not provided, or it is NULL the original | ||
| /// expression (1st argument) and return type is returned without | ||
| /// any modifications. | ||
| fn rewrite_null_expr_and_data_type( | ||
| args: &[Arc<dyn PhysicalExpr>], | ||
| expr_type: &DataType, | ||
| ) -> Result<(Arc<dyn PhysicalExpr>, DataType)> { | ||
| assert!(!args.is_empty()); | ||
| let expr = Arc::clone(&args[0]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets assert args are non empty before access the first element
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. Fixed. |
||
|
|
||
| // The input expression and the return is type is unchanged | ||
| // when the input expression is not NULL. | ||
| if !expr_type.is_null() { | ||
| return Ok((expr, expr_type.clone())); | ||
| } | ||
|
|
||
| get_scalar_value_from_args(args, 2)? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| .and_then(|value| { | ||
| ScalarValue::try_from(value.data_type().clone()) | ||
| .map(|sv| { | ||
| Ok(( | ||
| Arc::new(Literal::new(sv)) as Arc<dyn PhysicalExpr>, | ||
| value.data_type().clone(), | ||
| )) | ||
| }) | ||
| .ok() | ||
| }) | ||
| .unwrap_or(Ok((expr, expr_type.clone()))) | ||
| } | ||
|
|
||
| fn create_built_in_window_expr( | ||
| fun: &BuiltInWindowFunction, | ||
| args: &[Arc<dyn PhysicalExpr>], | ||
|
|
@@ -252,31 +287,35 @@ fn create_built_in_window_expr( | |
| } | ||
| } | ||
| BuiltInWindowFunction::Lag => { | ||
| let arg = Arc::clone(&args[0]); | ||
| // rewrite NULL expression and the return datatype | ||
| let (arg, out_data_type) = | ||
| rewrite_null_expr_and_data_type(args, out_data_type)?; | ||
| let shift_offset = get_scalar_value_from_args(args, 1)? | ||
| .map(get_signed_integer) | ||
| .map_or(Ok(None), |v| v.map(Some))?; | ||
| let default_value = | ||
| get_casted_value(get_scalar_value_from_args(args, 2)?, out_data_type)?; | ||
| get_casted_value(get_scalar_value_from_args(args, 2)?, &out_data_type)?; | ||
| Arc::new(lag( | ||
| name, | ||
| out_data_type.clone(), | ||
| default_value.data_type().clone(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: This works because 1st and 3rd arguments ends up having the same datatype after parsing the arguments above (for all cases). So passing 👍 |
||
| arg, | ||
| shift_offset, | ||
| default_value, | ||
| ignore_nulls, | ||
| )) | ||
| } | ||
| BuiltInWindowFunction::Lead => { | ||
| let arg = Arc::clone(&args[0]); | ||
| // rewrite NULL expression and the return datatype | ||
| let (arg, out_data_type) = | ||
| rewrite_null_expr_and_data_type(args, out_data_type)?; | ||
| let shift_offset = get_scalar_value_from_args(args, 1)? | ||
| .map(get_signed_integer) | ||
| .map_or(Ok(None), |v| v.map(Some))?; | ||
| let default_value = | ||
| get_casted_value(get_scalar_value_from_args(args, 2)?, out_data_type)?; | ||
| get_casted_value(get_scalar_value_from_args(args, 2)?, &out_data_type)?; | ||
| Arc::new(lead( | ||
| name, | ||
| out_data_type.clone(), | ||
| default_value.data_type().clone(), | ||
| arg, | ||
| shift_offset, | ||
| default_value, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4941,4 +4941,52 @@ NULL | |
| statement ok | ||
| DROP TABLE t; | ||
|
|
||
| ## end test handle NULL and 0 of NTH_VALUE | ||
| ## end test handle NULL and 0 of NTH_VALUE | ||
|
|
||
| ## test handle NULL of lead | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❤️ |
||
|
|
||
| statement ok | ||
| create table t1(v1 int); | ||
|
|
||
| statement ok | ||
| insert into t1 values (1); | ||
|
|
||
| query B | ||
| SELECT LEAD(NULL, 0, false) OVER () FROM t1; | ||
| ---- | ||
| NULL | ||
|
|
||
| query B | ||
| SELECT LAG(NULL, 0, false) OVER () FROM t1; | ||
| ---- | ||
| NULL | ||
|
|
||
| query B | ||
| SELECT LEAD(NULL, 1, false) OVER () FROM t1; | ||
| ---- | ||
| false | ||
|
|
||
| query B | ||
| SELECT LAG(NULL, 1, false) OVER () FROM t1; | ||
| ---- | ||
| false | ||
|
|
||
| statement ok | ||
| insert into t1 values (2); | ||
|
|
||
| query B | ||
| SELECT LEAD(NULL, 1, false) OVER () FROM t1; | ||
| ---- | ||
| NULL | ||
| false | ||
|
|
||
| query B | ||
| SELECT LAG(NULL, 1, false) OVER () FROM t1; | ||
| ---- | ||
| false | ||
| NULL | ||
|
|
||
| statement ok | ||
| DROP TABLE t1; | ||
|
|
||
| ## end test handle NULL of lead | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the naming its little bit abstract however its still good name but to help contributors understand what this method exactly doing lets add comments.
Having comments method increases chances that method can be reusable in future if the similar case shows up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯