uros-b commented on code in PR #59575:
URL: https://github.com/apache/spark/pull/59575#discussion_r4313238227
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionHelper.scala:
##########
@@ -204,6 +205,40 @@ abstract class TypeCoercionHelper {
.getOrElse(implicitCast(e, StringType).getOrElse(e))
}
+ /**
+ * Whether an implicit cast of `from` to `to` narrows a nanosecond-precision
timestamp, also
+ * inside an array, map or struct: a lower-precision timestamp drops
sub-microsecond digits and
+ * TIME drops the date part.
+ */
+ protected def isNanosNarrowing(from: DataType, to: DataType): Boolean =
+ existsNanosPair(from, to) { (nanos, t) =>
+ t.isInstanceOf[TimeType] ||
TimestampFamily.fractionalPrecision(nanos).exists(fp =>
+ TimestampFamily.fractionalPrecision(t).exists(_ < fp))
+ }
+
+ /**
+ * Picks the implicit cast target for a [[TypeCollection]] from the castable
member types, in
+ * member order. A nanosecond-precision timestamp, also nested, skips DATE,
which drops the time
+ * of day.
+ */
+ protected def pickTypeCollectionCast(
Review Comment:
The same DATE-fallback fix exists twice, with two different mechanisms.
#59578 rejects nanos in ApproximatePercentile / HistogramNumeric in two
steps: inputTypes returns the child's own nanos type, and a new
checkInputDataTypes branch rejects it. This PR produces the identical
UNEXPECTED_INPUT_TYPE generically, by skipping DATE in the TypeCollection
fallback.
If both land, one becomes dead weight.
- With this PR in, #59578's overrides never change the outcome.
- With #59578 in, this PR's ruleTest(new ApproximatePercentile(nanos, ...))
passes through the override instead of exercising pickTypeCollectionCast.
- Either way, the golden files carry the same percentile_approx /
approx_percentile / histogram_numeric cases twice.
- The generic rule also covers the internal ApproxCountDistinctForIntervals,
where TIMESTAMP precedes DATE: before this PR, non-ANSI picked TIMESTAMP and
ANSI picked DATE. #59578 doesn't touch it.
Ask: choose one mechanism, say so in both PR bodies, and drop the duplicate
golden cases from the other PR.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AnsiTypeCoercion.scala:
##########
@@ -204,9 +204,12 @@ object AnsiTypeCoercion extends TypeCoercionBase {
// When the target type is `TypeCollection`, there is another branch to
find the
// "closet convertible data type" below.
+ // Unlike store assignment, a nanos timestamp, also nested, is never
implicitly narrowed to
+ // TIME.
Review Comment:
The ANSI TIME block only applies at the top level, so nested TIME targets
still narrow nanos in ANSI mode while non-ANSI rejects them.
isNanosNarrowing only matches a bare nanos from. Take a concrete nested
target such as ArrayType(TimeType(6)):
- The ANSI arm calls Cast.canANSIStoreAssign(ArrayType(nanos),
ArrayType(TIME(6))), which recurses down to (_: DatetimeType, _: DatetimeType)
=> true (Cast.scala:514).
- isNanosNarrowing(ArrayType(..), ArrayType(..)) is false, so the cast is
inserted. Map values and struct fields behave the same way.
This is reachable through concrete nested parameter types:
- a typed Scala UDF taking Seq[java.time.LocalTime] (udfInputToCastType
yields ArrayType(TimeType(6), true));
- a procedure or DSv2 function parameter of type ARRAY<TIME>.
In ANSI mode, such an argument silently loses both the date and the
sub-microsecond digits. Non-ANSI rejects the same input, because its array/map
arms recurse into the guarded scalar arm. So the PR's goal that both modes
agree fails for nested inputs.
Nested precision narrowing is already blocked in ANSI, because
canANSIStoreAssign recurses into the SPARK-57303 check. Only TIME leaks.
Two possible fixes:
- Make isNanosNarrowing recurse through Array/Map/Struct (conf med that this
is sufficient). In ANSI this only adds the nested TIME case.
- Or move the TIME rule into canANSIStoreAssign, which already recurses.
This would also change INSERT into TIME columns, so it is a decision for the
nanos owners.
Either way, add ArrayType(nanos) -> ArrayType(TimestampType) /
ArrayType(TimeType()) cases to the cast matrix for both modes.
--
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]
