-
Notifications
You must be signed in to change notification settings - Fork 606
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
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)
@@ -12,6 +12,13 @@ | |||
|
|||
#[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
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
DATETIME
variant toDatetimeField
Custom
variant. 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