-
Notifications
You must be signed in to change notification settings - Fork 685
Support more DateTimeField variants
#1191
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 8407785234Details
💛 - Coveralls |
- Adds `DATETIME` variant to `DatetimeField` - Adds support for snowflake abbreviations via `Custom` variant. https://docs.snowflake.com/en/sql-reference/functions-date-time#supported-date-and-time-parts - Adds support for BigQuery weekday `WEEK(MONDAY)` syntax https://cloud.google.com/bigquery/docs/reference/standard-sql/date_functions#extract
0df4a25 to
cd640b9
Compare
alamb
left a comment
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.
Thank you @iffyio (and I very much apologize for the delay in reviewing). The only thing I think this PR needs is to avoid creating intermediate strings in the Display fmt
| }) | ||
| f.write_str( | ||
| match self { | ||
| DateTimeField::Year => "YEAR".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.
This change to call to_string() will now cause a new string to be allocated for each type.
I think it would be better / more effiicent to use write! directly -- like
| DateTimeField::Year => "YEAR".to_string(), | |
| match self { | |
| DateTimeField::Year => f.write_str("YEAR"), | |
| DateTimeField::Month => f.write_str("MONTH"), | |
| ... |
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.
Ah yes that makes a lot more sense, thanks for the follow up fix too!
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 anyone following along it was #1209)
| #[cfg(not(feature = "std"))] | ||
| use alloc::string::String; | ||
|
|
||
| #[cfg(not(feature = "std"))] |
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.
If you use the suggestion below I think you will not need these imports either
alamb
left a comment
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.
In order to keep the code flowing (and the fact it took me so long to review this PR) I fixed the string copy issue and pushed a fix to this PR.
Thanks again @iffyio
DATETIMEvariant toDatetimeFieldCustomvariant. https://docs.snowflake.com/en/sql-reference/functions-date-time#supported-date-and-time-partsWEEK(MONDAY)syntax https://cloud.google.com/bigquery/docs/reference/standard-sql/date_functions#extract