cloud-fan commented on code in PR #57008:
URL: https://github.com/apache/spark/pull/57008#discussion_r3812021829
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -4281,20 +4281,64 @@ case class ConvertTimezone(
Seq(
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true),
- TimestampNTZType)
- override def dataType: DataType = TimestampNTZType
-
- override def nullSafeEval(srcTz: Any, tgtTz: Any, micros: Any): Any = {
- DateTimeUtils.convertTimestampNtzToAnotherTz(
+ TypeCollection(TimestampNTZType, AnyTimestampNanoType))
+
+ // sourceTs's requiredType, as actually enforced by this method:
TypeCollection includes
+ // AnyTimestampNanoType (rather than an NTZ-only nanos type) only so that a
LTZ(p) input is
Review Comment:
**Nit:**
```suggestion
// AnyTimestampNanoType (rather than an NTZ-only nanos type) only so that
an LTZ(p) input is
```
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -2210,6 +2210,104 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-57818: convert_timezone over nanosecond-precision timestamps") {
+ val ntzType9 = TimestampNTZNanosType(9)
+
+ // The nanosWithinMicro remainder is carried through unchanged by a zone
conversion; only the
+ // whole-microsecond part shifts with the zone offset.
+ val srcNanos = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("2022-03-27T03:00:00.123456789"), 9)
+ val expectedNanos = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("2022-03-27T04:00:00.123456789"), 9)
+ assert(srcNanos.nanosWithinMicro == expectedNanos.nanosWithinMicro)
+ checkEvaluation(
+ ConvertTimezone(
+ Literal("Europe/Brussels"),
+ Literal("Europe/Moscow"),
+ Literal.create(srcNanos, ntzType9)),
+ expectedNanos)
+
+ // Pre-epoch values exercise the negative-epoch path. The expected
epochMicros is derived from
+ // the already-verified micros-only conversion (SPARK-37552 tests that
path); this only checks
+ // that the nanos wiring delegates to it correctly and carries the
remainder through unchanged.
+ val preEpochSrc = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("1960-01-01T00:00:00.000000001"), 9)
+ val preEpochExpectedMicros = DateTimeUtils.convertTimestampNtzToAnotherTz(
+ "Europe/Moscow", "Europe/Brussels", preEpochSrc.epochMicros)
+ checkEvaluation(
+ ConvertTimezone(
+ Literal("Europe/Moscow"),
+ Literal("Europe/Brussels"),
+ Literal.create(preEpochSrc, ntzType9)),
+ TimestampNanosVal.fromParts(preEpochExpectedMicros,
preEpochSrc.nanosWithinMicro))
+
+ // Precision (7/8/9) is preserved on the result;
AnyTimestampNanoType.defaultConcreteType
+ // would incorrectly always widen it to 9.
+ Seq(7, 8, 9).foreach { precision =>
+ val ntzType = TimestampNTZNanosType(precision)
+ val src = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("2022-03-27T03:00:00.123456789"), precision)
+ val convertExpr = ConvertTimezone(
+ Literal("Europe/Brussels"), Literal("Europe/Moscow"),
Literal.create(src, ntzType))
+ assert(convertExpr.dataType === ntzType)
+ }
+
+ // LTZ(p) nanos values are rejected: this function is NTZ-only, matching
the existing
+ // TimestampNTZType-only micro path. Unlike that micro path -- which does
implicit-cast a
Review Comment:
**Nit:**
```suggestion
// TimestampNTZType-only micro path. Unlike that micro path -- which
implicitly casts a
```
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -2210,6 +2210,104 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-57818: convert_timezone over nanosecond-precision timestamps") {
+ val ntzType9 = TimestampNTZNanosType(9)
+
+ // The nanosWithinMicro remainder is carried through unchanged by a zone
conversion; only the
+ // whole-microsecond part shifts with the zone offset.
+ val srcNanos = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("2022-03-27T03:00:00.123456789"), 9)
+ val expectedNanos = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("2022-03-27T04:00:00.123456789"), 9)
+ assert(srcNanos.nanosWithinMicro == expectedNanos.nanosWithinMicro)
+ checkEvaluation(
+ ConvertTimezone(
+ Literal("Europe/Brussels"),
+ Literal("Europe/Moscow"),
+ Literal.create(srcNanos, ntzType9)),
+ expectedNanos)
+
+ // Pre-epoch values exercise the negative-epoch path. The expected
epochMicros is derived from
+ // the already-verified micros-only conversion (SPARK-37552 tests that
path); this only checks
+ // that the nanos wiring delegates to it correctly and carries the
remainder through unchanged.
+ val preEpochSrc = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("1960-01-01T00:00:00.000000001"), 9)
+ val preEpochExpectedMicros = DateTimeUtils.convertTimestampNtzToAnotherTz(
+ "Europe/Moscow", "Europe/Brussels", preEpochSrc.epochMicros)
+ checkEvaluation(
+ ConvertTimezone(
+ Literal("Europe/Moscow"),
+ Literal("Europe/Brussels"),
+ Literal.create(preEpochSrc, ntzType9)),
+ TimestampNanosVal.fromParts(preEpochExpectedMicros,
preEpochSrc.nanosWithinMicro))
+
+ // Precision (7/8/9) is preserved on the result;
AnyTimestampNanoType.defaultConcreteType
+ // would incorrectly always widen it to 9.
+ Seq(7, 8, 9).foreach { precision =>
+ val ntzType = TimestampNTZNanosType(precision)
+ val src = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("2022-03-27T03:00:00.123456789"), precision)
+ val convertExpr = ConvertTimezone(
+ Literal("Europe/Brussels"), Literal("Europe/Moscow"),
Literal.create(src, ntzType))
+ assert(convertExpr.dataType === ntzType)
+ }
+
+ // LTZ(p) nanos values are rejected: this function is NTZ-only, matching
the existing
+ // TimestampNTZType-only micro path. Unlike that micro path -- which does
implicit-cast a
+ // plain LTZ TimestampType argument down to TimestampNTZType -- a nanos
source must not be
+ // silently reinterpreted from LTZ to NTZ, since that would drop the
source time zone
+ // information without the user asking for it.
+ val ltzNanos =
DateTimeUtils.instantToTimestampNanos(Instant.parse("2022-03-27T03:00:00Z"), 9)
+ val ltzMismatch = ConvertTimezone(
+ Literal("Europe/Brussels"), Literal("Europe/Moscow"),
+ Literal.create(ltzNanos, TimestampLTZNanosType(9)))
+ .checkInputDataTypes().asInstanceOf[DataTypeMismatch]
+ assert(ltzMismatch.errorSubClass == "UNEXPECTED_INPUT_TYPE")
+
+ // A wholly-invalid source type (not any kind of timestamp) hits the
generic type check
Review Comment:
**Nit:**
```suggestion
// A wholly invalid source type (not any kind of timestamp) hits the
generic type check
```
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -2210,6 +2210,104 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-57818: convert_timezone over nanosecond-precision timestamps") {
+ val ntzType9 = TimestampNTZNanosType(9)
+
+ // The nanosWithinMicro remainder is carried through unchanged by a zone
conversion; only the
+ // whole-microsecond part shifts with the zone offset.
+ val srcNanos = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("2022-03-27T03:00:00.123456789"), 9)
+ val expectedNanos = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("2022-03-27T04:00:00.123456789"), 9)
+ assert(srcNanos.nanosWithinMicro == expectedNanos.nanosWithinMicro)
+ checkEvaluation(
+ ConvertTimezone(
+ Literal("Europe/Brussels"),
+ Literal("Europe/Moscow"),
+ Literal.create(srcNanos, ntzType9)),
+ expectedNanos)
+
+ // Pre-epoch values exercise the negative-epoch path. The expected
epochMicros is derived from
+ // the already-verified micros-only conversion (SPARK-37552 tests that
path); this only checks
+ // that the nanos wiring delegates to it correctly and carries the
remainder through unchanged.
+ val preEpochSrc = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("1960-01-01T00:00:00.000000001"), 9)
+ val preEpochExpectedMicros = DateTimeUtils.convertTimestampNtzToAnotherTz(
+ "Europe/Moscow", "Europe/Brussels", preEpochSrc.epochMicros)
+ checkEvaluation(
+ ConvertTimezone(
+ Literal("Europe/Moscow"),
+ Literal("Europe/Brussels"),
+ Literal.create(preEpochSrc, ntzType9)),
+ TimestampNanosVal.fromParts(preEpochExpectedMicros,
preEpochSrc.nanosWithinMicro))
+
+ // Precision (7/8/9) is preserved on the result;
AnyTimestampNanoType.defaultConcreteType
+ // would incorrectly always widen it to 9.
+ Seq(7, 8, 9).foreach { precision =>
+ val ntzType = TimestampNTZNanosType(precision)
+ val src = DateTimeUtils.localDateTimeToTimestampNanos(
+ LocalDateTime.parse("2022-03-27T03:00:00.123456789"), precision)
+ val convertExpr = ConvertTimezone(
+ Literal("Europe/Brussels"), Literal("Europe/Moscow"),
Literal.create(src, ntzType))
+ assert(convertExpr.dataType === ntzType)
+ }
+
+ // LTZ(p) nanos values are rejected: this function is NTZ-only, matching
the existing
+ // TimestampNTZType-only micro path. Unlike that micro path -- which does
implicit-cast a
+ // plain LTZ TimestampType argument down to TimestampNTZType -- a nanos
source must not be
+ // silently reinterpreted from LTZ to NTZ, since that would drop the
source time zone
+ // information without the user asking for it.
+ val ltzNanos =
DateTimeUtils.instantToTimestampNanos(Instant.parse("2022-03-27T03:00:00Z"), 9)
+ val ltzMismatch = ConvertTimezone(
+ Literal("Europe/Brussels"), Literal("Europe/Moscow"),
+ Literal.create(ltzNanos, TimestampLTZNanosType(9)))
+ .checkInputDataTypes().asInstanceOf[DataTypeMismatch]
+ assert(ltzMismatch.errorSubClass == "UNEXPECTED_INPUT_TYPE")
+
+ // A wholly-invalid source type (not any kind of timestamp) hits the
generic type check
+ // instead of the explicit LTZ guard above; both paths must report the
same requiredType,
+ // since neither actually accepts a LTZ(p) source.
Review Comment:
**Nit:**
```suggestion
// since neither actually accepts an LTZ(p) source.
```
##########
sql/core/src/test/resources/sql-tests/inputs/timestamp-ntz-nanos.sql:
##########
@@ -150,6 +150,21 @@ SELECT TIMESTAMP_NTZ '1960-01-02 03:04:05.123456789' +
INTERVAL '0 00:00:00.0000
SELECT TIMESTAMP_NTZ '2020-01-02 03:04:05.123456789' + make_interval(0, 1, 0,
2, 0, 0, 0);
SELECT TIMESTAMP_NTZ '2020-01-02 03:04:05.123456789' + INTERVAL '1' MONTH;
+-- SPARK-57818: convert_timezone over nanosecond-precision TIMESTAMP_NTZ. The
sub-microsecond
+-- remainder is carried through unchanged; only the whole-microsecond part
shifts with the zone
+-- offset, and the result keeps the source's exact precision.
+SELECT convert_timezone('Europe/Brussels', 'Europe/Moscow',
+ TIMESTAMP_NTZ '2022-03-27 03:00:00.123456789');
+SELECT typeof(convert_timezone('Europe/Brussels', 'Europe/Moscow',
+ '2022-03-27 03:00:00.1234567' :: timestamp_ntz(7)));
+-- NULL nanosecond timestamp.
+SELECT convert_timezone('America/Los_Angeles', 'UTC', CAST(NULL AS
timestamp_ntz(9)));
+-- convert_timezone is NTZ-only; a nanosecond LTZ(p) source is rejected rather
than silently
+-- reinterpreted, unlike the microsecond path, which does implicit-cast a
plain LTZ TimestampType
+-- argument down to TIMESTAMP_NTZ.
+SELECT convert_timezone('Europe/Brussels', 'Europe/Moscow',
+ '2022-03-27 03:00:00.123456789 UTC' :: timestamp_ltz(9));
Review Comment:
**Non-blocking:**
This exact rejection case is also added to `timestamp-ltz-nanos.sql`, so it
generates duplicate analyzer and execution goldens. Please keep it in the LTZ
suite and let this suite focus on the positive NTZ paths.
##########
sql/core/src/test/resources/sql-tests/inputs/timestamp-ntz-nanos.sql:
##########
@@ -150,6 +150,21 @@ SELECT TIMESTAMP_NTZ '1960-01-02 03:04:05.123456789' +
INTERVAL '0 00:00:00.0000
SELECT TIMESTAMP_NTZ '2020-01-02 03:04:05.123456789' + make_interval(0, 1, 0,
2, 0, 0, 0);
SELECT TIMESTAMP_NTZ '2020-01-02 03:04:05.123456789' + INTERVAL '1' MONTH;
+-- SPARK-57818: convert_timezone over nanosecond-precision TIMESTAMP_NTZ. The
sub-microsecond
+-- remainder is carried through unchanged; only the whole-microsecond part
shifts with the zone
+-- offset, and the result keeps the source's exact precision.
+SELECT convert_timezone('Europe/Brussels', 'Europe/Moscow',
+ TIMESTAMP_NTZ '2022-03-27 03:00:00.123456789');
+SELECT typeof(convert_timezone('Europe/Brussels', 'Europe/Moscow',
+ '2022-03-27 03:00:00.1234567' :: timestamp_ntz(7)));
+-- NULL nanosecond timestamp.
+SELECT convert_timezone('America/Los_Angeles', 'UTC', CAST(NULL AS
timestamp_ntz(9)));
+-- convert_timezone is NTZ-only; a nanosecond LTZ(p) source is rejected rather
than silently
+-- reinterpreted, unlike the microsecond path, which does implicit-cast a
plain LTZ TimestampType
Review Comment:
**Nit:**
```suggestion
-- reinterpreted, unlike the microsecond path, which implicitly casts a
plain LTZ TimestampType
```
--
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]
