sunchao commented on code in PR #6740:
URL: https://github.com/apache/datafusion-comet/pull/6740#discussion_r4200348284
##########
native/spark-expr/src/kernels/temporal.rs:
##########
@@ -714,16 +731,20 @@ pub(crate) fn timestamp_trunc_dyn(
/// Keep key masking for fallible values and for coarse units with many
distinct values, where
/// masking can avoid expensive calendar work on unused entries. Fine
arithmetic processes the
/// physical values regardless of validity, so infallible fine units never
need a key scan.
+/// MICROSECOND is always infallible; SECOND and MILLISECOND are only while
they wrap.
fn timestamp_trunc_dictionary_needs_mask(
values: &dyn Array,
keys_len: usize,
format: &str,
+ wrap_second_millisecond_overflow: bool,
) -> Result<bool, SparkError> {
let Some(values) =
values.as_any().downcast_ref::<TimestampMicrosecondArray>() else {
return Ok(true);
};
let granularity = normalize_timestamp_trunc_format(format)?;
- if matches!(granularity, "microsecond" | "millisecond" | "second") {
+ if granularity == "microsecond"
+ || (wrap_second_millisecond_overflow && matches!(granularity,
"millisecond" | "second"))
Review Comment:
[P2] [P2] Preserve values-only truncation for safe non-UTC dictionaries
On Spark 4.2, with `spark.sql.session.timeZone=Asia/Tokyo` and
`spark.comet.expression.TruncTimestamp.allowIncompatible=true`, this condition
sends SECOND/MILLISECOND dictionaries into the unconditional non-UTC masking
branch below. Even ordinary, non-overflowing timestamps now allocate a mask and
scan every key. These units are timezone-independent, so safe inputs should
still require work proportional to the distinct values. A base/head kernel
comparison with 32 distinct timestamps measured approximately 4.9 µs versus
30.8 µs for 8,192 keys, and 4.8 µs versus 211.5 µs for 65,536 keys, with
identical results. Could the checked fine-unit path perform its value-range
check before the non-UTC guard, retaining key masking only when overflow is
possible?
Evidence: Reproduction: `/tmp/review-6740/kernel_bench.rs` compiles the
exact supplied base and head `temporal.rs` modules together using `rustc -O`
and identical dependency artifacts. Construct `DictionaryArray<Int32Type>` with
keys `i % 32`, values `1704067200123456 + j * 1234567` for j=0..31, and
timezone `Asia/Tokyo`. Compare base `timestamp_trunc_dyn(input, unit)` with
head `timestamp_trunc_dyn(input, unit, false)` for SECOND and MILLISECOND.
Results were asserted equal. Two alternating rounds of 2,000 calls each
consistently showed about 6x higher kernel time at 8,192 keys and 44x at 65,536
keys. UTC controls remained approximately unchanged. Timings use debug-built
dependencies and are not production throughput estimates. The source
establishes the cause: the new condition falls through to `return Ok(true)` for
non-UTC values, activating the full `array.keys_iter()` scan at lines 699–702.
`array_with_timezone` preserves dictionary encoding, making this reachable
through `
TimestampTruncExpr`.
--
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]
