dongjoon-hyun commented on code in PR #59571:
URL: https://github.com/apache/spark/pull/59571#discussion_r4314713772
##########
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:
Thank you. I updated the `TIME(7)`-`TIME(9)` case to truncate the value to
`t.precision` before formatting, like `formatNanos` does for the nanosecond
timestamps, and added test cases for `TIME(7)` and `TIME(8)` literals holding 9
fractional digits.
##########
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:
You're right. The root cause is `Literal(lt: LocalTime)` (and the Connect
`TIME` literal) building a `TIME(6)` value without truncation, which has been
rendered as `TIME '23:59:59.999999999'` since Apache Spark 4.1.0
(`function_lit.explain`). SPARK-57551 changed how this text is parsed back
(from `TIME(6)` truncated to microseconds to `TIME(9)`). Since fixing it
changes either the existing column names or the evaluated value of
`lit(LocalTime)`, let me handle it separately in a new JIRA issue.
##########
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:
Thank you, @MaxGekk and @peter-toth. As @peter-toth said, the `CAST`
argument is rejected with `INVALID_SQL_ARG`, but `spark.sql(args)` gets there
too when the argument is a session variable. With `DECLARE VARIABLE v3 TIME(3)
DEFAULT TIME '12:00:00.1'`, `SELECT v3 = '12:00:00.1001'` returns `true`, while
`spark.sql("SELECT :t = '12:00:00.1001', typeof(:t)", Map("t" -> expr("v3")))`
returns `false, time(6)`.
I agree that `TIME(0)`-`TIME(5)` arguments have been widened to `TIME(6)`
since 4.1, so let me handle it in a new JIRA issue together with the `TIME(6)`
case above, and I added a note to the PR description. One nuance from reading
the code: the comparison result changes only from 4.3 because SPARK-57551 made
`CAST(<string> AS TIME(p))` truncate to `p`. In 4.1 and 4.2,
`CAST('12:00:00.1001' AS TIME(3))` keeps the microseconds, so `v3 =
'12:00:00.1001'` returns `false` with or without the substitution.
##########
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:
With the truncation, `padToNanosPrecision` keeps its single invariant
(`fracLen <= precision`) again, so I restored its original comment. I kept
`Literal.toString` as is in this PR because truncating it also changes the
display of the existing `TIME(6)` literals with sub-microsecond digits (e.g.
`function_lit.explain`). It belongs to the new JIRA issue for the `TIME(6)`
case.
##########
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:
Added the over-precision case to `LiteralExpressionSuite` and the
ARRAY/STRUCT/MAP cases to `ParametersSuite`. The latter check the result types
because `spark.sql("SELECT array(TIME '12:00:00.123456789')").collect()` fails
with `CANNOT_UP_CAST_DATATYPE` even without parameters (`LocalTimeEncoder` is
fixed to `TimeType()`). Let me handle it in a new JIRA issue.
--
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]
