andygrove commented on code in PR #4915:
URL: https://github.com/apache/datafusion-comet/pull/4915#discussion_r3603914030
##########
native/spark-expr/src/kernels/temporal.rs:
##########
@@ -321,21 +322,48 @@ where
}
}
+/// Truncates a date expressed as days since the epoch, returning `None` if it
is out of range.
+type DateTruncFn = fn(i32) -> Option<i32>;
+
+/// The `date_trunc` formats Spark accepts, and the truncation each one
selects.
+const DATE_TRUNC_FORMATS: [(&str, DateTruncFn); 8] = [
+ ("YEAR", trunc_days_to_year),
+ ("YYYY", trunc_days_to_year),
+ ("YY", trunc_days_to_year),
+ ("QUARTER", trunc_days_to_quarter),
+ ("MONTH", trunc_days_to_month),
+ ("MON", trunc_days_to_month),
+ ("MM", trunc_days_to_month),
+ ("WEEK", trunc_days_to_week),
+];
+
+/// Resolve a `date_trunc` format string to the corresponding truncation
function.
+///
+/// The format is matched case-insensitively. Every supported format is ASCII,
so ASCII input can
+/// be compared in place, leaving the per-row format column path
allocation-free. Non-ASCII input
+/// still needs full Unicode case folding to fold the same way Spark does.
+fn date_trunc_fn_for_format(format: &str) -> Result<DateTruncFn, SparkError> {
+ let key: Cow<str> = if format.is_ascii() {
+ Cow::Borrowed(format)
+ } else {
+ Cow::Owned(format.to_uppercase())
+ };
+ DATE_TRUNC_FORMATS
+ .iter()
+ .find(|(name, _)| name.eq_ignore_ascii_case(&key))
+ .map(|(_, trunc_fn)| *trunc_fn)
+ .ok_or_else(|| {
+ SparkError::Internal(format!(
+ "Unsupported format: {format:?} for function 'date_trunc'"
+ ))
+ })
+}
Review Comment:
Dropped the `Cow` / `is_ascii()` split. Since every entry in
`DATE_TRUNC_FORMATS` is ASCII and `eq_ignore_ascii_case` only folds ASCII, a
non-ASCII input can never match either way — the fallback allocation was pure
cost.
##########
native/spark-expr/src/kernels/temporal.rs:
##########
Review Comment:
Extended the table pattern to `ntz_trunc_fn_for_format` and added a
monomorphized-for-`DateTime<Tz>` sibling `tz_trunc_fn_for_format`. Both the tz
array-format helper and the scalar-format tz path now resolve via
`eq_ignore_ascii_case` against a static table, so the per-row and
per-invocation `to_uppercase()` allocations are gone.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
