andygrove commented on code in PR #5135:
URL: https://github.com/apache/datafusion-comet/pull/5135#discussion_r3683663588
##########
native/spark-expr/src/hash_funcs/utils.rs:
##########
@@ -706,6 +706,23 @@ macro_rules! create_hashes_internal {
$hash_method
);
}
+
DataType::Interval(arrow::datatypes::IntervalUnit::MonthDayNano) => {
+ let array = col
+ .as_any()
+ .downcast_ref::<IntervalMonthDayNanoArray>()
+ .unwrap();
+ for (hash, value) in
$hashes_buffer.iter_mut().zip(array.iter()) {
+ if let Some(value) = value {
+ // Match Spark's generated hash code, which omits
the days field:
+ //
https://github.com/apache/spark/blob/710b3c45aab88bd14e51d49f400e2f31e3b65772/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala#L420-L423
Review Comment:
Good find on the field order, but the link needs a rethink. `710b3c45` is a
master snapshot, and Spark master changed this eight days ago in SPARK-58236
(`a2616be0f`, 2026-07-22, "Fix hash codegen dropping the CalendarInterval days
field"), which adds the missing `days` round to the codegen path so it matches
the interpreted path. So the linked lines no longer say what the comment says
they say.
The PR is still correct for everything we support: `days` is omitted in
`v3.4.3`, `v3.5.8`, `v4.0.1`, `v4.1.2`, `v4.2.0` and on `branch-4.0` /
`branch-4.1` / `branch-4.2`, and the fix has not been backported.
Could we point at a release tag such as `v4.2.0` instead of a master SHA,
and name SPARK-58236 in the comment so the next person understands this becomes
version-dependent once we support a Spark that carries the fix? Worth filing a
follow-up issue for that too. Per our convention the version switch would live
in `ShimCometConf` and travel through proto rather than becoming a native shim.
##########
native/spark-expr/src/hash_funcs/utils.rs:
##########
@@ -706,6 +706,23 @@ macro_rules! create_hashes_internal {
$hash_method
);
}
+
DataType::Interval(arrow::datatypes::IntervalUnit::MonthDayNano) => {
+ let array = col
Review Comment:
Would you mind pulling this into a `hash_array_interval_month_day_nano!`
macro alongside the other `hash_array_*` macros at the top of the file? The
neighbours all split a `null_count() == 0` fast path from the null-checking
one, and they use the `unwrap_or_else(|| panic!("Failed to downcast column to
{}. Actual data type: {:?}", ...))` form rather than a bare `.unwrap()`, which
gives a much better message if a downcast ever goes wrong. This hasher runs on
the shuffle and join hot path, so the fast path is worth having.
Small related nit: if you add `IntervalUnit` to the `use
arrow::datatypes::{DataType, TimeUnit};` inside the macro, the match arm can
read `DataType::Interval(IntervalUnit::MonthDayNano)` like the `TimeUnit` arms
above it.
##########
spark/src/test/scala/org/apache/comet/CometHashExpressionSuite.scala:
##########
@@ -128,6 +130,24 @@ class CometHashExpressionSuite extends CometTestBase with
AdaptiveSparkPlanHelpe
}
}
+ test("hash - calendar interval") {
Review Comment:
Could this go in a SQL file test instead? There is already a
`sql-tests/expressions/datetime/calendar_interval.sql` using the `VALUES` plus
`make_interval` pattern with `spark.comet.exec.localTableScan.enabled=true`,
and a `sql-tests/expressions/hash/hash.sql`, so either would host it. That
would also cover the exact repro from #5059 (`SELECT hash(make_interval(y, m,
0, d, h, 0, 0)) FROM t`), which the current test does not go through.
##########
spark/src/test/scala/org/apache/comet/CometHashExpressionSuite.scala:
##########
@@ -128,6 +130,24 @@ class CometHashExpressionSuite extends CometTestBase with
AdaptiveSparkPlanHelpe
}
}
+ test("hash - calendar interval") {
+ withTempView("t") {
+ val rows = Seq(
+ Row(new CalendarInterval(14, 25, 18367008009L)),
+ Row(new CalendarInterval(0, 1, 0L)),
+ Row(new CalendarInterval(0, 2, 0L)),
+ Row(new CalendarInterval(-14, -25, -18367008009L)),
+ Row(null))
Review Comment:
Good set of values, especially the days-only pair that pins the omitted
field. A few more worth adding.
An all-zero interval is the one I would most want, because
`Murmur3_x86_32.hashInt(0, seed) != seed`, so a zero interval still shifts the
seed chain and that is exactly where a wrong fold order would show up.
Then months-only and microseconds-only, a non-default seed such as `hash(c,
1)` and `hash(c, 0)`, and a multi-argument form like `hash(c, someInt)` to
exercise the seed chaining across columns.
And since the list and struct paths recurse back into this macro,
`hash(array(c))` and `hash(struct(c))` should work now too, so a query for
those would lock that in.
--
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]
