uros-b commented on code in PR #58880:
URL: https://github.com/apache/spark/pull/58880#discussion_r4080905550
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -3011,6 +3011,65 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-57833: timestampdiff over nanosecond-precision timestamps") {
+ val sec = 1000000L // microseconds per second
+ def ntz(micros: Long, frac: Int, p: Int = 9): Literal =
+ Literal(TimestampNanosVal.fromParts(micros, frac.toShort),
TimestampNTZNanosType(p))
+ def ltz(micros: Long, frac: Int, p: Int = 9): Literal =
+ Literal(TimestampNanosVal.fromParts(micros, frac.toShort),
TimestampLTZNanosType(p))
+
+ // NANOSECOND unit reports the exact sub-microsecond difference (within a
microsecond, across
+ // microseconds, and negative).
+ checkEvaluation(TimestampDiff("NANOSECOND", ntz(0, 100), ntz(0, 900)),
800L)
+ checkEvaluation(TimestampDiff("NANOSECOND", ntz(0, 900), ntz(2, 100)),
1200L)
+ checkEvaluation(TimestampDiff("NANOSECOND", ntz(2, 100), ntz(0, 900)),
-1200L)
+
+ // The sub-microsecond fraction participates in the truncated count for
coarser units too: a
+ // start fraction larger than the end fraction means a whole
second/microsecond has NOT elapsed.
+ // SECOND: 1s + 100ns - 900ns = 0.9999992s -> 0 (the micros-only count
would wrongly be 1).
+ checkEvaluation(TimestampDiff("SECOND", ntz(0, 900), ntz(sec, 100)), 0L)
+ // 1s + 900ns - 100ns = 1.0000008s -> 1.
+ checkEvaluation(TimestampDiff("SECOND", ntz(0, 100), ntz(sec, 900)), 1L)
+ // MICROSECOND: 2100ns - 900ns = 1200ns -> 1 (the micros-only count would
wrongly be 2).
+ checkEvaluation(TimestampDiff("MICROSECOND", ntz(0, 900), ntz(2, 100)), 1L)
+
+ // Precision 7 (100ns step) and 8 (10ns step) fractions.
+ checkEvaluation(TimestampDiff("NANOSECOND", ntz(0, 100, 7), ntz(0, 300,
7)), 200L)
+ checkEvaluation(TimestampDiff("NANOSECOND", ntz(0, 110, 8), ntz(0, 200,
8)), 90L)
+
+ // LTZ (zone-aware), exact whole minute with equal fractions.
+ checkEvaluation(
+ TimestampDiff("MINUTE", ltz(0, 500), ltz(60 * sec, 500), Some("UTC")),
1L)
+
+ // Mixed operands: a microsecond TIMESTAMP_LTZ start (zero fraction) and a
nanosecond LTZ end.
+ checkEvaluation(
+ TimestampDiff("SECOND", Literal(0L, TimestampType), ltz(sec, 500),
Some("UTC")), 1L)
+
+ // NANOSECOND between two microsecond timestamps is well-defined
(fractions are zero): the
+ // difference is a whole number of microseconds times 1000.
+ checkEvaluation(
+ TimestampDiff("NANOSECOND", Literal(0L, TimestampType), Literal(1L,
TimestampType)), 1000L)
+
+ // Null propagation on either operand.
+ checkEvaluation(
+ TimestampDiff("SECOND", Literal.create(null, TimestampNTZNanosType(9)),
ntz(sec, 0)), null)
+ checkEvaluation(
+ TimestampDiff("NANOSECOND", ntz(0, 1), Literal.create(null,
TimestampNTZNanosType(9))), null)
+
+ // A NANOSECOND difference wider than ~292 years overflows a 64-bit
nanosecond count. It is
+ // surfaced as DATETIME_OVERFLOW (matching the timestampadd side) rather
than a raw
+ // ArithmeticException, on both the microsecond and the nanosecond-carrier
code paths.
+ checkErrorInExpression[SparkArithmeticException](
+ TimestampDiff("NANOSECOND",
+ Literal(-5000000000000000L, TimestampType), Literal(5000000000000000L,
TimestampType)),
+ condition = "DATETIME_OVERFLOW",
+ parameters = Map("operation" -> "get the number of NANOSECOND between
the two timestamps"))
+ checkErrorInExpression[SparkArithmeticException](
+ TimestampDiff("NANOSECOND", ntz(-5000000000000000L, 0),
ntz(5000000000000000L, 0)),
+ condition = "DATETIME_OVERFLOW",
+ parameters = Map("operation" -> "get the number of NANOSECOND between
the two timestamps"))
+ }
+
Review Comment:
DateExpressionsSuite.scala / new golden queries -- test coverage gap that
hid the regression: every added case pairs operands of the same zone family
(NTZ+NTZ, LTZ+LTZ, and micros-LTZ + nanos-LTZ). There is no mixed NTZ/LTZ case
(micros or nanos) and no DATE+NTZ case -- exactly the combinations broken
above. Add them; they pin the correct pre-existing values and will fail until
the zone-handling concern is fixed.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -5179,27 +5184,73 @@ case class TimestampDiff(
override def left: Expression = startTimestamp
override def right: Expression = endTimestamp
- override def inputTypes: Seq[AbstractDataType] = Seq(TimestampType,
TimestampType)
+ // Micro-precision NTZ operands keep coercing to TIMESTAMP (LTZ), preserving
the pre-nanos
+ // timestampdiff semantics for TIMESTAMP_NTZ; only the new
nanosecond-precision types are accepted
+ // natively (there is no prior behavior to preserve for them).
+ override def inputTypes: Seq[AbstractDataType] =
Review Comment:
TimestampDiff.inputTypes -- widening both operands from Seq(TimestampType,
TimestampType) to Seq(TypeCollection(AnyTimestampType, AnyTimestampNanoType),
...) silently regresses existing microsecond mixed-zone results. On master both
operands were unified to TimestampType (LTZ), so the single zoneIdInEval =
zoneIdForType(endTimestamp) applied to both was safe. With the wider accepted
set, AnyTimestampType.acceptsType lets an NTZ operand stay NTZ (and a DATE
coerces to LTZ via defaultConcreteType = TimestampType), so operands of
different zone families reach the eval, which then applies one end-derived zone
to both -- double/mis-applying the offset. Confirmed against master + CI:
timestamp-ntz.sql q#20 timestampdiff(HOUR, timestamp_ntz'...01:02:03',
timestamp_ltz'...02:03:04') now returns 9 (master golden: 1), and the PR's own
regenerated timestampNTZ/timestamp.sql.out bakes in timestampdiff(SECOND,
date'2022-02-15', ntz'...23:59:59') = -28801 (correct: -1; -28801 = the -8h
session
offset applied to the NTZ/DATE operand, minus 1s). The new timestampDiffNanos
helper shares the identical single-zoneId-for-both design, so mixed
nanos-NTZ/nanos-LTZ operands have the same latent bug (untested). Feasible
direction: derive the zone per operand (zoneIdForType(startTimestamp.dataType)
/ ...(endTimestamp.dataType)) instead of one shared zone -- traced to restore 1
for q#20 and to leave same-family cases unchanged; exact fix should be
validated against the full golden suite (fix conf:med, concern conf:high).
--
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]
