cloud-fan commented on code in PR #57085:
URL: https://github.com/apache/spark/pull/57085#discussion_r3595819693
##########
docs/sql-migration-guide.md:
##########
@@ -29,6 +29,7 @@ license: |
- Since Spark 4.3, the Spark Thrift Server rejects setting JVM system
properties through the `set:system:` session configuration overlay (for
example, in a JDBC connection string). To restore the previous behavior, set
`spark.sql.legacy.hive.thriftServer.allowSettingSystemProperties` to `true`.
- Since Spark 4.3, the adaptive execution rule
`org.apache.spark.sql.execution.adaptive.DynamicJoinSelection` has been renamed
to `DemoteBroadcastHashJoin`, which now only demotes broadcast hash joins
(emitting `NO_BROADCAST_HASH`). Its selection of shuffled hash join over sort
merge join has moved to a new physical rule gated by
`spark.sql.adaptive.convertSortMergeJoinToShuffledHashJoin.enabled` (default
`true`). If you previously disabled the shuffled-hash-join preference by
listing `org.apache.spark.sql.execution.adaptive.DynamicJoinSelection` in
`spark.sql.adaptive.optimizer.excludedRules`, that name no longer matches any
rule (unknown names are silently ignored); set
`spark.sql.adaptive.convertSortMergeJoinToShuffledHashJoin.enabled` to `false`
instead.
- Since Spark 4.3, the exact `percentile`, `percentile_cont`, and `median`
aggregate functions (including their `WITHIN GROUP (ORDER BY ...)` forms)
compute the linear interpolation between two neighboring values as `lower +
fraction * (higher - lower)` instead of `(1 - fraction) * lower + fraction *
higher`. The two are equal in exact arithmetic, but the new form is
monotonically non-decreasing in the requested percentage and avoids a rounding
error the old form could introduce. As a result these functions may return a
value that differs from earlier releases in the last ULP. `percentile_disc` and
`percentile_approx` are unaffected.
Review Comment:
The prior-behavior claim here is not correct, and it changes what this entry
should say. `TimestampToLongBase` extends `ExpectsInputTypes` (not
`ImplicitCastInputTypes`), so the analyzer never implicitly cast a
`TIMESTAMP_NTZ` argument to `TIMESTAMP_LTZ`. With the old `inputTypes =
Seq(TimestampType)`, a `TIMESTAMP_NTZ` input was **rejected** with
`DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE` (verified in both ANSI and non-ANSI
modes). So there was no prior (zone-shifted) result, and the `CAST(... AS
TIMESTAMP_LTZ)` remediation reproduces nothing.
This makes the change purely additive: previously-rejected types are now
accepted. A pure capability addition normally doesn't need an "Upgrading" entry
at all, since it can't change the result of any query that previously worked.
If you keep a note, suggest reframing it as additive:
```suggestion
- Since Spark 4.3, `unix_seconds`, `unix_millis`, and `unix_micros` accept
`TIMESTAMP_NTZ` and the nanosecond-precision timestamp types directly, reading
them with no time-zone shift. Previously these functions accepted only
`TIMESTAMP_LTZ`; a `TIMESTAMP_NTZ` or nanosecond-timestamp argument was
rejected with a `DATATYPE_MISMATCH` error. This is a new capability and does
not change the result of any query that previously succeeded.
```
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -1739,6 +1739,50 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
checkEvaluation(UnixMicros(Literal(timestampWithNanos)), 1000001L)
}
+ test("SPARK-57814: unix_seconds / unix_millis / unix_micros over " +
+ "NTZ and nanosecond-precision timestamps") {
+ import org.apache.spark.sql.catalyst.util.TimestampNanosTestUtils._
+
+ // 2008-12-25 15:30:00.123456789 -> epochMicros 1230219000123456 (+789 ns
remainder dropped).
+ // unix_* apply no zone shift, so the NTZ wall-clock value and the LTZ
instant at the same UTC
+ // reading produce identical results.
+ val ntz = localDateTimeToNanosVal(timestampNTZ(2008, 12, 25, 15, 30, 0,
123456789))
+ val ltz =
instantToNanosVal(Instant.parse("2008-12-25T15:30:00.123456789Z"))
+ foreachNanosPrecision { p =>
+ checkEvaluation(UnixSeconds(Literal.create(ntz,
TimestampNTZNanosType(p))), 1230219000L)
+ checkEvaluation(UnixSeconds(Literal.create(ltz,
TimestampLTZNanosType(p))), 1230219000L)
+ checkEvaluation(UnixMillis(Literal.create(ntz,
TimestampNTZNanosType(p))), 1230219000123L)
+ checkEvaluation(UnixMillis(Literal.create(ltz,
TimestampLTZNanosType(p))), 1230219000123L)
+ checkEvaluation(UnixMicros(Literal.create(ntz,
TimestampNTZNanosType(p))), 1230219000123456L)
+ checkEvaluation(UnixMicros(Literal.create(ltz,
TimestampLTZNanosType(p))), 1230219000123456L)
+ }
+
+ // Micro TIMESTAMP_NTZ is now accepted directly and read as a physical
Long with no zone shift.
+ // It was NOT rejected before this change: implicit coercion silently cast
it to TIMESTAMP_LTZ,
Review Comment:
This comment's premise is incorrect (same root as the migration-guide note):
micro `TIMESTAMP_NTZ` was **not** accepted-via-silent-coercion before — it was
rejected with `DATATYPE_MISMATCH`, because `TimestampToLongBase` uses
`ExpectsInputTypes`, which does no implicit casting. So there was no "old
shift" to reintroduce, and the `Cast(microNtz, TimestampType, LA)` lines are
not "the equivalent old coercion."
The assertions are fine to keep — they're a useful contrast showing that
reading NTZ directly (zoneless) differs from explicitly casting it to
`TIMESTAMP_LTZ` under a non-UTC zone. Please just fix the comment so it doesn't
record the false prior-behavior story, e.g. describe the explicit-cast lines as
a contrast between direct (zoneless) NTZ reads and an explicit `TIMESTAMP_LTZ`
cast, rather than as the previous implicit behavior.
--
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]
