MaxGekk commented on code in PR #59571:
URL: https://github.com/apache/spark/pull/59571#discussion_r4302997546
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala:
##########
@@ -688,6 +689,10 @@ case class Literal (value: Any, dataType: DataType)
extends LeafExpression {
case (v: Decimal, t: DecimalType) => s"${v}BD"
case (v: Int, DateType) =>
s"DATE '$toString'"
+ // toString strips trailing zeros, but the parser derives TIME(7)..TIME(9)
from the number of
+ // fractional digits (6 or fewer give TIME(6)), so pad back to exactly
`precision` digits.
+ case (_: Long, t: TimeType) if t.precision > TimeType.MICROS_PRECISION =>
+ s"TIME '${padToNanosPrecision(toString, t.precision)}'"
case (_: Long, _: TimeType) =>
Review Comment:
The TIME(6) branch has the same issue. `Literal(lt: LocalTime)` at line 92
creates `TimeType()` from `localTimeToNanos` without truncating, so
`LocalTime.of(23, 59, 59, 999999999)` prints 9 digits and parses back as
TIME(9). There is a witness in
`sql/connect/common/src/test/resources/query-tests/explain-results/function_lit.explain`,
which shows a TIME(6) literal as `TIME '23:59:59.999999999'`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala:
##########
@@ -642,9 +642,10 @@ case class Literal (value: Any, dataType: DataType)
extends LeafExpression {
ts + "." + "0" * precision
} else {
val fracLen = ts.length - dotIdx - 1
- // fracLen can never exceed precision:
formatNanos/formatWithoutTimeZoneNanos truncate
- // the value to `precision` digits before formatting, and the
fractionFormatter only
- // strips trailing zeros (never adds digits); the else branch handles
fracLen == precision.
+ // fracLen can never exceed precision for the nanosecond timestamps:
formatNanos/
Review Comment:
This comment documents the root cause rather than fixing it: `toString` for
`TimeType` calls `new FractionTimeFormatter().format(nanos)` with no precision,
unlike the nanosecond timestamps where the formatter truncates to `precision`.
If `toString` truncated to `t.precision` as well, the helper could keep its
single invariant (`fracLen <= precision`), and the two TIME cases below could
collapse into one precision-aware case.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala:
##########
@@ -688,6 +689,10 @@ case class Literal (value: Any, dataType: DataType)
extends LeafExpression {
case (v: Decimal, t: DecimalType) => s"${v}BD"
case (v: Int, DateType) =>
s"DATE '$toString'"
+ // toString strips trailing zeros, but the parser derives TIME(7)..TIME(9)
from the number of
+ // fractional digits (6 or fewer give TIME(6)), so pad back to exactly
`precision` digits.
+ case (_: Long, t: TimeType) if t.precision > TimeType.MICROS_PRECISION =>
+ s"TIME '${padToNanosPrecision(toString, t.precision)}'"
case (_: Long, _: TimeType) =>
s"TIME '$toString'"
Review Comment:
TIME(0)-TIME(5) do not survive the round trip either, and padding cannot
help there because 6 or fewer digits always give TIME(6). A TIME(3) argument
such as `expr("CAST(TIME '12:00:00.1' AS TIME(3))")` becomes `TIME
'12:00:00.1'`, so `SELECT :t = '12:00:00.1005'` compares at microsecond
precision and returns false, while at TIME(3) the string is truncated to `.100`
and the result is true.
If we want the type to be preserved for every precision, one option is to
emit `CAST(TIME '<value>' AS TIME(p))` whenever `p` differs from what the
parser would infer from the digits. If that is out of scope here, it is
probably worth a note in the PR description.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala:
##########
@@ -688,6 +689,10 @@ case class Literal (value: Any, dataType: DataType)
extends LeafExpression {
case (v: Decimal, t: DecimalType) => s"${v}BD"
case (v: Int, DateType) =>
s"DATE '$toString'"
+ // toString strips trailing zeros, but the parser derives TIME(7)..TIME(9)
from the number of
+ // fractional digits (6 or fewer give TIME(6)), so pad back to exactly
`precision` digits.
+ case (_: Long, t: TimeType) if t.precision > TimeType.MICROS_PRECISION =>
Review Comment:
This pads a short fraction but does not cut a long one.
`Literal.create(LocalTime.of(12, 0, 0, 123456789), TimeType(7))` is accepted
(validation only checks for a `Long`, and neither `TimeConverter` nor
`localTimeToNanos` truncates), `toString` prints all 9 digits,
`padToNanosPrecision` returns them as is, and the parser infers TIME(9). So
`spark.sql("SELECT :t", Map("t" -> thatLiteral))` still comes back as TIME(9)
instead of TIME(7).
I think the simplest fix is to truncate before formatting, e.g.
`DateTimeUtils.truncateTimeToPrecision(value, t.precision)`, which is what
`formatNanos` already does for the nanosecond timestamps.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/LiteralExpressionSuite.scala:
##########
@@ -717,6 +717,22 @@ class LiteralExpressionSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-60314: TimeType(7-9) sql keeps the precision on a round trip") {
Review Comment:
All of these values have exactly `p` fractional digits, so the
over-precision case (`Literal(nanos with 9 digits, TimeType(7))`) is not
exercised and would pass today. Could you add a case for that, and maybe one
for ARRAY/STRUCT/MAP of TIME(9), which are rendered element-wise via
`Literal(_, elementType).sql` during parameter substitution?
--
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]
