cloud-fan commented on code in PR #56969:
URL: https://github.com/apache/spark/pull/56969#discussion_r4323425325
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -3051,6 +3051,8 @@ case class TimestampAddYMInterval(
3.94959677
> SELECT _FUNC_('1997-02-28 10:30:00', '1996-10-30', false);
3.9495967741935485
+ > SELECT _FUNC_(TIMESTAMP_NTZ '1997-02-28 10:30:00.123456789',
TIMESTAMP_NTZ '1996-10-30 00:00:00');
Review Comment:
**Nit (P3):** With `spark.sql.timestampNanosTypes.enabled` off (the
production default), the parser treats this as a microsecond `TIMESTAMP_NTZ`
literal and drops the trailing `789`. So in a default session, the documented
example doesn't actually show nanosecond input. It produces a nanosecond
literal in `ExpressionInfoSuite` only because tests enable the flag. Every
other nanosecond example in this file enables the flag first:
```suggestion
> SET spark.sql.timestampNanosTypes.enabled=true;
spark.sql.timestampNanosTypes.enabled true
> SELECT _FUNC_(TIMESTAMP_NTZ '1997-02-28 10:30:00.123456789',
TIMESTAMP_NTZ '1996-10-30 00:00:00');
```
This extends an [existing
discussion](https://github.com/apache/spark/pull/56969#discussion_r3983010805).
**Verification:**
- **Inspection:** The months_between description shows the preview-flag SET
line immediately before its nanosecond example, matching the sibling nanosecond
examples.
- **Behavior:** The function example checks still run every months_between
example and produce the documented outputs.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -774,6 +774,118 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-57819: months_between over nanosecond-precision timestamps") {
+ val ntzType = TimestampNTZNanosType(9)
+ val ltzType = TimestampLTZNanosType(9)
+
+ // A nanos operand is reduced to its epochMicros, so the result must match
the
+ // microsecond-only result exactly, regardless of the nanosWithinMicro
remainder.
+ val ntz1 = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("1997-02-28T10:30:00"), 9)
+ val ntz2 = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("1996-10-30T00:00:00"), 9)
+ checkEvaluation(
+ MonthsBetween(Literal.create(ntz1, ntzType), Literal.create(ntz2,
ntzType),
+ Literal.TrueLiteral, Some("UTC")),
+ 3.94959677)
+ checkEvaluation(
+ MonthsBetween(Literal.create(ntz1, ntzType), Literal.create(ntz2,
ntzType),
+ Literal.FalseLiteral, Some("UTC")),
+ 3.9495967741935485)
+
+ val ltz1 =
DateTimeUtils.instantToTimestampNanos(Instant.parse("1997-02-28T10:30:00Z"), 9)
+ val ltz2 =
DateTimeUtils.instantToTimestampNanos(Instant.parse("1996-10-30T00:00:00Z"), 9)
+ checkEvaluation(
+ MonthsBetween(Literal.create(ltz1, ltzType), Literal.create(ltz2,
ltzType),
+ Literal.TrueLiteral, Some("UTC")),
+ 3.94959677)
+ checkEvaluation(
+ MonthsBetween(Literal.create(ltz1, ltzType), Literal.create(ltz2,
ltzType),
+ Literal.FalseLiteral, Some("UTC")),
+ 3.9495967741935485)
+
+ // A non-zero nanosWithinMicro remainder must not perturb the result: only
epochMicros
+ // reaches the computation, at both the coarse (whole-day) and fine
(whole-second) branches.
+ val ntz1WithNanos = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("1997-02-28T10:30:00.000000999"), 9)
Review Comment:
**Nit (P3):** This input can't tell truncation from rounding.
`10:30:00.000000999` rounds to `10:30:00.000001`, which is still the same whole
second, so `MICROSECONDS.toSeconds` gives the same `secondsInDay`. The second
case (`2015-01-30 11:52:00.000000999` vs `11:50:00`) takes the
same-day-of-month early return, which ignores time of day entirely. So it
doesn't exercise the whole-second branch that the comment above claims. The
`.000000500` golden cases have the same gap.
Inputs at a boundary would make the claim falsifiable. For example,
`1997-02-28T10:30:59.999999999` with `roundOff = false` gives
`3.949618802270012` when truncated but `3.94961917562724` if rounded. A
`23:59:59.999999999` operand would move to the next day if rounded. Asserting
that each result equals the result for the microsecond-truncated value would
catch a rounding change.
**Verification:**
- **Regression:** Cover a nanosecond operand at xx:59.999999999 on the
fractional branch and at 23:59:59.999999999 next to a day boundary. Assert that
each result equals the result for the microsecond-truncated operand, so that
rounding instead of truncation would fail.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -3072,24 +3074,46 @@ case class MonthsBetween(
override def second: Expression = date2
override def third: Expression = roundOff
- override def inputTypes: Seq[AbstractDataType] = Seq(TimestampType,
TimestampType, BooleanType)
+ // Nanosecond-precision timestamps are accepted alongside the microsecond
types. The result is a
+ // fraction of a month derived from the whole-day and whole-second parts of
each operand, so the
+ // sub-microsecond remainder cannot move it and each operand contributes
only its epochMicros.
+ override def inputTypes: Seq[AbstractDataType] = Seq(
+ TypeCollection(AnyTimestampType, AnyTimestampNanoType),
+ TypeCollection(AnyTimestampType, AnyTimestampNanoType),
+ BooleanType)
override def dataType: DataType = DoubleType
override def withTimeZone(timeZoneId: String): TimeZoneAwareExpression =
copy(timeZoneId = Option(timeZoneId))
+ @transient private lazy val zoneIdInEval: ZoneId =
zoneIdForType(date1.dataType)
Review Comment:
**Blocking (P1):** `zoneIdInEval` depends only on `date1`'s family, and both
`nullSafeEval` and `doGenCode` pass both operands' micros to
`DateTimeUtils.monthsBetween` with that one zone. That is correct only when
both operands are in the same family, and nothing guarantees that anymore:
- `inputTypes` accepts microsecond `TIMESTAMP_NTZ` without a cast through
`AnyTimestampType`, while `DATE` and `STRING` operands are still implicitly
cast to `TIMESTAMP`, its default concrete type.
- Unlike `SubtractTimestamps`, `MonthsBetween` has no case in
`DateTimeOperationsTypeCoercion` / `AnsiDateTimeOperationsTypeCoercion` that
unifies a cross-family pair.
Before this PR, both slots were `TimestampType`, so every NTZ, DATE, and
STRING operand was cast to `TIMESTAMP`, and both operands were read in the
session zone. With microsecond inputs only (no preview flag involved), in a
non-UTC session:
| Session | Query | Before | Now |
|---|---|---|---|
| America/Los_Angeles | `months_between(timestamp_ntz'1997-02-28 10:30:00',
timestamp_ltz'1996-10-30 00:00:00')` | 3.94959677 | 3.93884409 |
| America/Los_Angeles | same pair with `timestamp_ltz` first | 3.94959677 |
3.96034946 |
| Asia/Tokyo | `months_between(timestamp_ntz'2015-07-31 00:00:00',
date'2015-02-28')` | 5.0 | 5.10887097 |
In the last row, the DATE is cast to LTZ midnight JST and then read in UTC.
It becomes Feb 27, so the month-end rule no longer applies. `months_between(a,
b)` is no longer `-months_between(b, a)`, and existing queries and views over
`TIMESTAMP_NTZ` columns change results without any signal. The same mixing
applies to the new `TIMESTAMP_NTZ(p)` operands paired with an LTZ, DATE, or
STRING operand.
The new tests lock this behavior in instead of guarding against it:
- `results/timestamp-ntz.sql.out` records the two order-dependent values
above as expected.
- `timestamp-ntz-nanos.sql` does the same for an NTZ(9)/LTZ pair.
- The mixed-family checks in `DateExpressionsSuite` run only under UTC,
where a zone taken from `date1` alone cannot diverge.
- No test pairs NTZ with DATE or STRING.
**Recommended change:** Add a months_between case to both the ANSI and the
non-ANSI datetime operand coercions. When the two timestamp operands come from
different families, cast the TIMESTAMP_NTZ-family operand to the TIMESTAMP_LTZ
type of the same precision (TIMESTAMP for microseconds, TIMESTAMP_LTZ(p) for
nanoseconds), so both operands are evaluated in the session time zone as
microsecond inputs were before this change. Update the months_between comments
in the SQL test inputs, regenerate the affected golden files, and add datetime
expression and SQL tests for mixed-family pairs in a non-UTC session.
**Why this works:** months_between reads both operands with the zone of the
first operand's family, which is correct only when both operands share a
family. Unifying a mixed pair into the TIMESTAMP_LTZ family during analysis
restores that precondition before evaluation. Both operands are then read in
the session time zone, exactly as with the old implicit cast to TIMESTAMP, and
the result no longer depends on argument order. Same-family pairs are
untouched: TIMESTAMP_LTZ pairs keep the session zone and TIMESTAMP_NTZ pairs
keep UTC.
**Scope:** Unify mixed-family months_between operands into the TIMESTAMP_LTZ
family during analysis, and add tests and regenerated golden files that pin
consistent, order-independent results.
**Compatibility:** These stay as they are: TIMESTAMP-only months_between
plans and results, same-family pair semantics, and the rule that a nanosecond
operand's sub-microsecond digits never change the result.
**Risks:** Analyzed plans for mixed-family months_between gain an explicit
cast on the TIMESTAMP_NTZ-family operand, so the analyzer golden outputs for
those queries change. A TIMESTAMP_NTZ value that falls in a session-zone DST
gap is shifted forward by the cast exactly as the old implicit cast did, so
this is the previous behavior rather than a new edge case.
**Constraints:** Keep the ANSI and non-ANSI coercion cases identical, so
both modes produce the same plan and result. Fire only when both operands are
timestamp-family types from different families, and leave same-family pairs,
DATE and STRING handling, and NULL operands to the existing implicit casts.
**Exclusions:**
- This repair keeps same-family behavior as it is in the PR. Whether
same-family microsecond TIMESTAMP_NTZ pairs should be evaluated in UTC rather
than the session zone (which changes results on DST-transition days) is left to
the author's answer to the separate question about that behavior.
**Success:** In a non-UTC session, months_between on a microsecond
TIMESTAMP_NTZ value paired with a TIMESTAMP_LTZ, DATE, or STRING operand
returns the same value as before this PR. Examples: 3.94959677 for
TIMESTAMP_NTZ '1997-02-28 10:30:00' and TIMESTAMP_LTZ '1996-10-30 00:00:00' in
America/Los_Angeles, and 5.0 for TIMESTAMP_NTZ '2015-07-31 00:00:00' and DATE
'2015-02-28' in Asia/Tokyo. For every mixed-family pair, including nanosecond
operands, swapping the arguments negates the result, up to rounding. The golden
outputs no longer record argument-order-dependent values for mixed-family pairs.
**Verification:**
- **Regression:** Cover months_between in non-UTC sessions such as
America/Los_Angeles and Asia/Tokyo for a microsecond TIMESTAMP_NTZ operand
paired with TIMESTAMP_LTZ, DATE, and STRING operands, in both argument orders.
Assert the pre-PR values and that swapping the arguments negates the result.
- **Behavior:** Cover nanosecond TIMESTAMP_NTZ(p) and TIMESTAMP_LTZ(p)
operands paired with an operand of the other family, including DATE and STRING,
in a non-UTC session and in both argument orders. Assert the value obtained by
reading both operands in the session time zone, under both ANSI and non-ANSI
coercion.
- **Compatibility:** Confirm that TIMESTAMP-only inputs and same-family
TIMESTAMP_NTZ and TIMESTAMP_LTZ pairs, both microsecond and nanosecond, return
unchanged results. Confirm that the analyzed plan adds a cast only to the
TIMESTAMP_NTZ-family operand of a mixed pair, in both the iterative analyzer
and the single-pass resolver.
--
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]
