MaxGekk commented on code in PR #56455:
URL: https://github.com/apache/spark/pull/56455#discussion_r3399385206
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala:
##########
@@ -562,13 +570,30 @@ case class Literal (value: Any, dataType: DataType)
extends LeafExpression {
ExprCode.forNonNullValue(JavaCode.expression(s"($javaType)$value",
dataType))
case TimestampType | TimestampNTZType | LongType | _:
DayTimeIntervalType | _: TimeType =>
toExprCode(s"${value}L")
+ case (_: TimestampNTZNanosType | _: TimestampLTZNanosType)
+ if TypeOps(dataType).isDefined =>
Review Comment:
Is this case needed in this PR? The default branch below already generates
correct code for nanos literals: `javaType` resolves to `TimestampNanosVal`
(`CodeGenerator.scala:2036`) and the class is `Serializable`, so
`addReferenceObj` works. That is also how every other object-valued literal
(`UTF8String`, `Decimal`, `CalendarInterval`) is generated, and it loads one
reference object instead of re-evaluating `fromParts(...)` at every use site of
the literal's value (per row in a comparison, for example).
I'd remove this case from this PR and open a sub-task under
[SPARK-53504](https://issues.apache.org/jira/browse/SPARK-53504) to route
literal codegen through `TypeOps.getJavaLiteral` in a generic way — note
`TimeType` literals don't route through it either, so a generic solution would
cover both.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala:
##########
@@ -562,13 +570,30 @@ case class Literal (value: Any, dataType: DataType)
extends LeafExpression {
ExprCode.forNonNullValue(JavaCode.expression(s"($javaType)$value",
dataType))
case TimestampType | TimestampNTZType | LongType | _:
DayTimeIntervalType | _: TimeType =>
toExprCode(s"${value}L")
+ case (_: TimestampNTZNanosType | _: TimestampLTZNanosType)
+ if TypeOps(dataType).isDefined =>
+ ExprCode.forNonNullValue(
+ JavaCode.expression(TypeOps(dataType).get.getJavaLiteral(value),
dataType))
case _ =>
val constRef = ctx.addReferenceObj("literal", value, javaType)
ExprCode.forNonNullValue(JavaCode.global(constRef, dataType))
}
}
}
+ private def padToNanosPrecision(ts: String, precision: Int): String = {
+ val dotIdx = ts.indexOf('.')
+ if (dotIdx < 0) {
+ 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 is a
defensive fallback.
Review Comment:
This reads as if the else branch is rarely taken, but it's the normal path
whenever `fracLen == precision` (any full-precision literal); only `fracLen >
precision` is impossible.
```suggestion
// strips trailing zeros (never adds digits); the else branch handles
fracLen == precision.
```
--
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]
