sunchao commented on code in PR #5292:
URL: https://github.com/apache/datafusion-comet/pull/5292#discussion_r4105000296
##########
native/spark-expr/src/datetime_funcs/make_interval.rs:
##########
@@ -16,72 +16,225 @@
// under the License.
use crate::arithmetic_overflow_error;
-use arrow::array::Array;
-use arrow::datatypes::DataType;
-use datafusion::common::Result;
-use datafusion::logical_expr::{ColumnarValue, ScalarFunctionArgs,
ScalarUDFImpl, Signature};
-use datafusion_spark::function::datetime::make_interval::SparkMakeInterval as
DataFusionMakeInterval;
+use arrow::array::{Array, ArrayRef, Decimal128Array, Int32Array, Int64Array,
StructArray};
+use arrow::buffer::NullBuffer;
+use arrow::datatypes::{DataType, Field, Fields};
+use datafusion::common::{DataFusionError, Result};
+use datafusion::logical_expr::{
+ ColumnarValue, ScalarFunctionArgs, ScalarUDFImpl, Signature, Volatility,
+};
+use std::collections::HashMap;
+use std::sync::Arc;
+
+const CALENDAR_INTERVAL_STRUCT_KEY: &str = "SPARK::calendarInterval::struct";
+const MICROS_PER_HOUR: i64 = 3_600_000_000;
+const MICROS_PER_MINUTE: i64 = 60_000_000;
+
+pub fn calendar_interval_type() -> DataType {
+ let months = Field::new("months", DataType::Int32,
false).with_metadata(HashMap::from([(
+ CALENDAR_INTERVAL_STRUCT_KEY.to_string(),
+ "true".to_string(),
+ )]));
+ DataType::Struct(Fields::from(vec![
Review Comment:
[P2] Keep calendar intervals out of the generic struct hashing path.
Returning this struct also changes how the existing native `hash` and
`xxhash64` kernels process intervals: they hash children in struct order and
ignore the logical-type marker. For a Parquet table `t(y INT)` containing `1`,
`SELECT hash(make_interval(y)), xxhash64(make_interval(y)) FROM t` should match
Spark. The new representation instead produces two different values without an
error. This materially worsens the previous unsupported-type failure into
silent incorrect results. Add marker-aware Spark-compatible handling, or route
calendar-interval hashes through Spark until that handling exists.
Evidence: With constant folding excluded, Spark 4.1.3 evaluated the
equivalent query over `range(1,2)` as `(-351543533, 604378839101286624)`. A
disposable native test passed HEAD `SparkMakeInterval(1,0,0,0,0,0,0)` directly
to HEAD `spark_murmur3_hash` and `spark_xxhash64`, obtaining `(-912233426,
3333565817687609978)`. The unchanged hash macro recursively hashes ordinary
struct children. Running it on the previous `IntervalMonthDayNano(12,0,0)`
representation instead returned `Unsupported data type in hasher:
Interval(MonthDayNano)`, confirming that the silent wrong-result behavior comes
from this representation change.
##########
spark/src/main/scala/org/apache/spark/sql/comet/util/Utils.scala:
##########
@@ -215,12 +220,55 @@ object Utils extends CometTypeShim with Logging {
.add(MapVector.VALUE_NAME, valueType, nullable =
valueContainsNull),
nullable = false,
timeZoneId)).asJava)
+ case CalendarIntervalType =>
+ val fieldType = new FieldType(nullable, ArrowType.Struct.INSTANCE,
null)
+ val monthsType = new FieldType(
+ false,
+ new ArrowType.Int(32, true),
+ null,
+ Map(calendarIntervalStructKey -> "true").asJava)
+ new Field(
+ name,
+ fieldType,
+ Seq(
+ new Field("months", monthsType, Seq.empty[Field].asJava),
+ new Field(
+ "days",
+ new FieldType(false, new ArrowType.Int(32, true), null),
+ Seq.empty[Field].asJava),
+ new Field(
+ "microseconds",
+ new FieldType(false, new ArrowType.Int(64, true), null),
+ Seq.empty[Field].asJava)).asJava)
case dataType =>
val fieldType = new FieldType(nullable, toArrowType(dataType,
timeZoneId), null)
new Field(name, fieldType, Seq.empty[Field].asJava)
}
}
+ /**
+ * Returns true only for the Spark-tagged `CalendarIntervalType` struct
produced by
+ * [[toArrowField]]. Matching the field shape (`months: Int32`, `days:
Int32`, `microseconds:
+ * Int64`, all non-nullable) is not sufficient: a user-defined
`struct<months:int, days:int,
+ * microseconds:bigint>` has the same shape, so this also requires the
+ * `SPARK::calendarInterval::struct` metadata marker on the `months` child.
Only fields carrying
+ * the marker round-trip back to `CalendarIntervalType`; unmarked structs
stay plain
+ * `StructType`s.
+ */
+ def isCalendarIntervalStructField(field: Field): Boolean = {
+ val children = field.getChildren
+ def child(index: Int, name: String, bits: Int): Boolean = {
+ val f = children.get(index)
+ f.getName == name && f.getType == new ArrowType.Int(bits, true) &&
!f.isNullable
Review Comment:
[P2] Preserve interval recognition after aggregate nullability
normalization. The new `!f.isNullable` requirement rejects schemas produced by
`coerce_collect_child_nullability`: native `collect_list` makes all three
interval children nullable while retaining the marker. With native aggregation
and codegen enabled, `SELECT transform(collect_list(make_interval(y)), x -> x)
FROM t`, where `t.y` contains `1`, should return `[1 years]`. Instead, the
aggregate output is classified as `array<struct<...>>`, so its codegen consumer
lacks `getInterval` and throws. The previous interval representation survives
this path. Keep tagged intervals atomic during normalization, or make
recognition tolerate this widening, and cover aggregate-to-codegen consumption.
Evidence: Ran the HEAD `SparkMakeInterval` kernel, the exact planner
nullability helper and `CometCollectList`, then exported the actual aggregate
result through Arrow IPC. Its marked children were nullable. HEAD
`Utils.fromArrowField` returned `ArrayType(StructType(...),true)`, and the
unchanged HEAD codegen implementation evaluating Spark `ArrayTransform` threw
`UnsupportedOperationException: InputArray_col0: getInterval not implemented
for this array shape`. The equivalent previous `IntervalMonthDayNano`
representation was recognized as `ArrayType(CalendarIntervalType,true)` and
completed successfully. Spark 4.1.3 returned `[1 years]`.
--
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]
