-
Notifications
You must be signed in to change notification settings - Fork 603
Support BigQuery window function null treatment #1239
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
Conversation
Pull Request Test Coverage Report for Build 8900668299Details
💛 - Coveralls |
30627b0
to
e4eeee9
Compare
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.
My reading of the referenced docs don't seem to permit syntax like FIRST_VALUE(x) IGNORE NULLS OVER
Are we sure this is supported by BigQuery?
FIRST_VALUE (value_expression [{RESPECT | IGNORE} NULLS])
OVER over_clause
over_clause:
{ named_window | ( [ window_specification ] ) }
window_specification:
[ named_window ]
[ PARTITION BY partition_expression [, ...] ]
ORDER BY expression [ { ASC | DESC } ] [, ...]
[ window_frame_clause ]
@alamb yeah so bigquery doesn't support |
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.
Are we sure this is supported by BigQuery?
@alamb yeah so bigquery doesn't support
FIRST_VALUE(x) IGNORE NULLS OVER
, that one we have support for in the parser today - e.g snowflake uses that variant. This one adds support for the variant linked in the doc, soFIRST_VALUE(x IGNORE NULLS) OVER
with similar example. Does that clarify the difference in behavior?
Ah, I see now. BigQuery / Snowflake, why!!!! LOL
Overall this looks good to me -- I have two small suggestions but we could do them as a follow on PR too if you prefer.
Thank you @iffyio
src/ast/mod.rs
Outdated
/// The declaration occurs after the function call. | ||
/// | ||
/// ```sql | ||
/// FIRST_VALUE(x IGNORE NULLS) OVER () |
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.
This looks like a copy paste error?
/// FIRST_VALUE(x IGNORE NULLS) OVER () |
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.
Fixed!
pub over: Option<WindowType>, | ||
// aggregate functions may specify eg `COUNT(DISTINCT x)` | ||
/// aggregate functions may specify eg `COUNT(DISTINCT x)` |
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 driveby cleanup
src/ast/mod.rs
Outdated
match self.null_treatment { | ||
Some(NullTreatmentType::FunctionArg(null_treatment)) => { | ||
format!(" {null_treatment}") | ||
} | ||
_ => "".to_string(), | ||
} |
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.
I think you can avoid this seeming allocation in "".to_string()
by using Cow
:
match self.null_treatment { | |
Some(NullTreatmentType::FunctionArg(null_treatment)) => { | |
format!(" {null_treatment}") | |
} | |
_ => "".to_string(), | |
} | |
match self.null_treatment { | |
Some(NullTreatmentType::FunctionArg(null_treatment)) => { | |
Cow::Owned(format!(" {null_treatment}")) | |
} | |
_ => Cow::Borrowed("") | |
} |
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.
Done!
Syntax differs for BigQuery on positioning of the `IGNORE|RESPECT NULL` clause within a window function. This extends the parser to cover that syntax. https://cloud.google.com/bigquery/docs/reference/standard-sql/navigation_functions#first_value
e4eeee9
to
cddb87d
Compare
Thanks @iffyio |
Sorry for posting this comment post-merge, but I was looking at fixing up our support for aggs that use the
@alamb @iffyio wdyt? If we do this, we should do it before the next release to avoid API churn... |
That sounds reasonable to me! |
Syntax differs for BigQuery on positioning of the
IGNORE|RESPECT NULL
clause within a window function. This extends the parser to cover that syntax.https://cloud.google.com/bigquery/docs/reference/standard-sql/navigation_functions#first_value