uros-b commented on code in PR #57681:
URL: https://github.com/apache/spark/pull/57681#discussion_r3700478253
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/types/ops/TimestampNanosOrcOps.scala:
##########
@@ -69,6 +79,19 @@ case class TimestampLTZNanosOrcOps(t: TimestampLTZNanosType)
extends OrcTypeOps
val instant = ts.toInstant
set(ordinal, DateTimeUtils.instantToTimestampNanos(instant, t.precision))
}
+
+ // The physical ORC category is a timestamp, so the search argument uses the
TIMESTAMP leaf type.
+ override def predicateLeafType: Option[PredicateLeaf.Type] =
Some(PredicateLeaf.Type.TIMESTAMP)
+
+ // The filter literal is an external java.time.Instant (see
CatalystTypeConverters). ORC evaluates
+ // a TIMESTAMP_INSTANT predicate against the stored UTC value shifted into
the JVM default zone,
+ // so the literal must be a java.sql.Timestamp whose local wall clock equals
the instant's UTC
+ // wall clock; Timestamp.valueOf(LocalDateTime at UTC) produces exactly
that. Any non-Instant
+ // value is passed through unchanged.
+ override def castFilterLiteral(value: Any): Any = value match {
+ case i: Instant => Timestamp.valueOf(LocalDateTime.ofInstant(i,
ZoneOffset.UTC))
Review Comment:
@stevomitric Please check: castFilterLiteral for TimestampLTZNanosType uses
Timestamp.valueOf(LocalDateTime.ofInstant(i, ZoneOffset.UTC)), which produces a
java.sql.Timestamp whose getTime() (epoch millis) equals the epoch of the UTC
wall clock interpreted as the JVM local zone. Because
java.sql.Timestamp.compareTo / getTime() are strictly epoch-millis-based (not
"local wall clock"-based), and because OrcTimestamp for a TIMESTAMP_INSTANT
column stores instant.toEpochMilli() (true UTC epoch), these two epoch-millis
values diverge for any non-UTC JVM timezone. In UTC-ahead zones (e.g. IST,
UTC+5:30) the filter literal is smaller than the stored epoch, so ORC's
SearchArgument will skip stripes that contain valid matching rows; data loss.
In UTC-behind zones stripes are over-included (conservative, not data-lossy).
The correct construction is new java.sql.Timestamp(i.toEpochMilli) with
setNanos(i.getNano()), matching how toJavaTimestampNoRebase and the regular
TimestampType filter pat
h work. The PR description's "shifted into the JVM default zone" reasoning is
incorrect: Timestamp.compareTo operates on epoch millis regardless of any JVM
zone shift.
--
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]
