sunchao commented on code in PR #57088:
URL: https://github.com/apache/spark/pull/57088#discussion_r3540463537
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/arrow/ArrowWriterSuite.scala:
##########
@@ -323,6 +323,118 @@ class ArrowWriterSuite extends SparkFunSuite {
}
}
+ test("calendar interval overflow raises DATETIME_OVERFLOW at the conversion
site") {
+ // The default IntervalMonthDayNano mapping multiplies microseconds by
1000 into Arrow's
+ // int64 nanosecond field. The overflow must surface as the structured
DATETIME_OVERFLOW
+ // (not a raw ArithmeticException), and the translation must be scoped to
the conversion:
+ // an unrelated (Spark)ArithmeticException raised by upstream evaluation
must pass through
+ // unchanged, which the writer guarantees by catching only around
Math.multiplyExact.
Review Comment:
[P3] Could we add the regression case for the upstream-exception half of
this invariant? This test currently checks only the `Math.multiplyExact`
overflow. If a future refactor widened the `try` to include
`input.getInterval`, an upstream `DIVIDE_BY_ZERO` could be relabeled as
`DATETIME_OVERFLOW` while this test stayed green. A custom `InternalRow` whose
`getInterval` throws a `SparkArithmeticException`, followed by an assertion
that the original exception escapes unchanged, would pin the behavior.
##########
sql/api/src/main/scala/org/apache/spark/sql/util/ArrowUtils.scala:
##########
@@ -437,6 +478,23 @@ private[sql] object ArrowUtils {
}
}
+ /**
+ * Whether the Arrow struct field is the lossless representation of a
CalendarInterval built by
+ * `toArrowField` with `losslessInternalTypes = true`. Also callable from
Java
+ * (ArrowColumnVector) to select the interval accessor for such structs.
+ */
+ def isCalendarIntervalStructField(field: Field): Boolean = {
+ field.getType.isInstanceOf[ArrowType.Struct] &&
+ field.getChildren.asScala
+ .map(_.getName)
+ .asJava
+ .containsAll(Seq("months", "days", "microseconds").asJava) &&
+ field.getChildren.asScala.exists { child =>
+ child.getName == "months" &&
+ child.getMetadata.getOrDefault(calendarIntervalStructKey, "false") ==
"true"
Review Comment:
[P3] Could this recognize only the exact canonical shape: exactly three
children in `(months, days, microseconds)` order with `int32`, `int32`, and
`int64` types and the expected nullability? A tagged reordered schema such as
`(days, months, microseconds)` passes this predicate today.
`CalendarIntervalStructWriter` writes children positionally while
`ArrowColumnVector` reads them by name, so that shape silently swaps months and
days. The current producer is canonical; this is defensive hardening for
corrupt or future-evolved tagged schemas.
--
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]
