This is an automated email from the ASF dual-hosted git repository.
MaxGekk pushed a commit to branch branch-4.x
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-4.x by this push:
new 7d0a167933ac [MINOR][SQL] Make TypeOps extend TypeApiOps in the Types
Framework
7d0a167933ac is described below
commit 7d0a167933ac57207797af4517d7fd475cb97c77
Author: David Milicevic <[email protected]>
AuthorDate: Wed Jun 17 17:26:00 2026 +0200
[MINOR][SQL] Make TypeOps extend TypeApiOps in the Types Framework
### What changes were proposed in this pull request?
A small type-system refactor of the Types Framework, plus the zone-handling
fix it surfaces.
**Trait lift.** The server-side `TypeOps` trait now extends the api-side
`TypeApiOps` trait instead of `Serializable`:
```scala
-trait TypeOps extends Serializable {
- /** The DataType this Ops instance handles. */
- def dataType: DataType
+trait TypeOps extends TypeApiOps {
```
Concrete `*TypeOps` classes already extend their matching `*TypeApiOps`
(e.g. `TimeTypeOps extends TimeTypeApiOps(t) with TypeOps`), so this change
has no effect on any
existing implementation -- it only lifts that relationship to the trait
level. As a result the
duplicate `def dataType: DataType` declaration on `TypeOps` is removed (it
is inherited from
`TypeApiOps`), and the class doc / implementation guidance are updated to
match.
**Follow-on: TIMESTAMP_LTZ nanos render zone.** Advertising the client-side
operations (`format`,
`toSQLValue`, ...) as directly callable on a `TypeOps` value means the
server-side
`TimestampLTZNanosTypeOps` must carry a real rendering zone. It previously
hard-coded
`ZoneOffset.UTC`, which was sound only under the older "server-side ops are
never used for
rendering" invariant. The follow-up commit changes it to default to the
session-local time zone,
passed by-name so the parent builds its formatter lazily -- the config is
read only if a value is
actually rendered through the server-side ops, never on the physical-type /
literal / row-accessor
/ serde paths. This touches three files beyond the trait:
`TimestampNanosTypeApiOps` (by-name
`zoneId`), `TimestampNanosTypeOps` (session-zone default), and
`ToStringBase` (codegen
serialization hygiene for the threaded zone).
### Why are the changes needed?
The framework factory `TypeOps.apply(dt)` returns `Option[TypeOps]`, so
call sites manipulate
values whose static type is `TypeOps`. The client-side operations
(`format`, `toSQLValue`,
`getEncoder`, ...) are declared on `TypeApiOps`. Before this change,
invoking a client-side
operation on a value known only as `TypeOps` required a runtime
`isInstanceOf[TypeApiOps]`
narrowing, which is sound only by the convention that every concrete
`*TypeOps` happens to extend a
`*TypeApiOps` -- nothing in the type system enforces it.
Making `TypeOps` extend `TypeApiOps`:
- lets callers holding an `Option[TypeOps]` invoke client-side methods
directly via `.map`, without
an unsafe downcast;
- turns "every server-side `TypeOps` is also a client-side `TypeApiOps`"
into a compile-time
guarantee rather than a runtime invariant -- a new `*TypeOps` that does
not provide the
client-side operations will not compile;
- removes the now-redundant `dataType` declaration that both traits
previously carried.
This is a forward-looking modeling change: as more server-side integration
points need client-side
operations on a `TypeOps` value, they can call them directly and safely.
### Does this PR introduce _any_ user-facing change?
No observable change. The trait lift is compile-time-only. The follow-on
commit corrects the zone
the server-side `TypeOps` would use to render TIMESTAMP_LTZ nanos (UTC ->
session-local), but no
production path renders LTZ nanos through the server-side ops today, so
there is no behavioral
difference in practice. The correction makes the newly-documented "invoke
client-side methods on a
`TypeOps` value" contract accurate for the point at which such a caller is
added. Both
CAST-to-string paths (interpreted and codegen) already thread the cast's
own resolved zone, so cast
output is unchanged.
### How was this patch tested?
The trait lift is covered by compilation and the existing Types Framework
suites. A new test in
`TimestampNanosTypeOpsSuite` pins the zone correction: under a non-UTC
session zone
(`America/Los_Angeles`) it asserts the server-side
`TimestampLTZNanosTypeOps` renders TIMESTAMP_LTZ
in the session-local zone, so a regression back to a fixed UTC zone would
fail it.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code
This pull request and its description were written by Isaac.
Closes #56449 from davidm-db/typeops_extends_typeapiops.
Authored-by: David Milicevic <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
(cherry picked from commit 64ceedcd43df02462fdc43f7809ad063f3b8b5dc)
Signed-off-by: Max Gekk <[email protected]>
---
.../spark/sql/types/ops/TimestampNanosTypeApiOps.scala | 10 ++++++----
.../spark/sql/catalyst/expressions/ToStringBase.scala | 5 ++++-
.../sql/catalyst/types/ops/TimestampNanosTypeOps.scala | 14 +++++++++-----
.../apache/spark/sql/catalyst/types/ops/TypeOps.scala | 15 ++++++++++-----
.../catalyst/types/ops/TimestampNanosTypeOpsSuite.scala | 16 ++++++++++++++++
5 files changed, 45 insertions(+), 15 deletions(-)
diff --git
a/sql/api/src/main/scala/org/apache/spark/sql/types/ops/TimestampNanosTypeApiOps.scala
b/sql/api/src/main/scala/org/apache/spark/sql/types/ops/TimestampNanosTypeApiOps.scala
index 4e050b69471a..011e5504e96e 100644
---
a/sql/api/src/main/scala/org/apache/spark/sql/types/ops/TimestampNanosTypeApiOps.scala
+++
b/sql/api/src/main/scala/org/apache/spark/sql/types/ops/TimestampNanosTypeApiOps.scala
@@ -124,12 +124,14 @@ class TimestampNTZNanosTypeApiOps(val t:
TimestampNTZNanosType) extends Timestam
* @param t
* The TimestampLTZNanosType with precision information
* @param zoneId
- * The time zone LTZ values are rendered in (LTZ is zone-aware).
`TypeApiOps.apply` threads in
- * the session zone: the cast's resolved zone for CAST, or the session-local
time zone config
- * for zone-less render callers (Row JSON via formatExternal).
+ * The time zone LTZ values are rendered in (LTZ is zone-aware). Passed
by-name and forced only
+ * when the formatter is first built, so non-rendering callers never
evaluate it. The factories
+ * thread it in: `TypeApiOps.apply` passes the cast's resolved zone (CAST)
or the session-local
+ * time zone (zone-less callers such as Row JSON); the catalyst
`TimestampLTZNanosTypeOps`
+ * passes the session-local time zone for the server-side `TypeOps` path.
* @since 4.3.0
*/
-class TimestampLTZNanosTypeApiOps(val t: TimestampLTZNanosType, zoneId: ZoneId)
+class TimestampLTZNanosTypeApiOps(val t: TimestampLTZNanosType, zoneId: =>
ZoneId)
extends TimestampNanosTypeApiOps {
override def dataType: TimestampLTZNanosType = t
override protected def sqlTypeName: String = "TIMESTAMP_LTZ"
diff --git
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala
index 65e0b125c225..56e494ced8fe 100644
---
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala
+++
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala
@@ -242,7 +242,10 @@ trait ToStringBase { self: UnaryExpression with
TimeZoneAwareExpression =>
// Route nanosecond timestamp cast-to-string through the Types
Framework: emit a runtime
// call into the ops reference object. The cast's session zone is
threaded into the lookup
// so LTZ carries it; NTZ is zone-independent (SPARK-57285).
- val ops = TypeApiOps(from, zoneId).get
+ // Resolve the zone here so the reference object holds a ZoneId, not a
closure capturing
+ // this Cast; the held value is the cast's resolved zone, not a
session-config read.
+ val z = zoneId
+ val ops = TypeApiOps(from, z).get
// Pin the reference-object cast type to the public TypeApiOps class;
the runtime ops class
// lives in sql/api, so the inferred concrete-class cast would be
unnecessarily specific.
val opsRef = JavaCode.global(
diff --git
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimestampNanosTypeOps.scala
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimestampNanosTypeOps.scala
index 96fe32999b93..ca3976528b25 100644
---
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimestampNanosTypeOps.scala
+++
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimestampNanosTypeOps.scala
@@ -17,7 +17,7 @@
package org.apache.spark.sql.catalyst.types.ops
-import java.time.{Instant, LocalDateTime, ZoneOffset}
+import java.time.{Instant, LocalDateTime}
import org.apache.spark.SparkIllegalArgumentException
import org.apache.spark.sql.catalyst.InternalRow
@@ -25,6 +25,7 @@ import org.apache.spark.sql.catalyst.expressions.{Expression,
Literal, MutableTi
import org.apache.spark.sql.catalyst.expressions.objects.StaticInvoke
import org.apache.spark.sql.catalyst.types.{PhysicalDataType,
PhysicalTimestampLTZNanosType, PhysicalTimestampNTZNanosType}
import org.apache.spark.sql.catalyst.util.DateTimeUtils
+import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types.{ObjectType, TimestampLTZNanosType,
TimestampNTZNanosType}
import org.apache.spark.sql.types.ops.{TimestampLTZNanosTypeApiOps,
TimestampNTZNanosTypeApiOps}
import org.apache.spark.unsafe.types.TimestampNanosVal
@@ -130,11 +131,14 @@ case class TimestampNTZNanosTypeOps(override val t:
TimestampNTZNanosType)
* The TimestampLTZNanosType with precision information
* @since 4.3.0
*/
-// Server-side TypeOps is used only for physical type, literals, row
accessors, and serde - never
-// for rendering (cast-to-string flows through TypeApiOps.apply). So the
rendering zone is unused
-// here; pass UTC rather than reading the session config on every construction.
+// LTZ is zone-aware, so the inherited renderer needs a zone. The server-side
TypeOps has none of
+// its own (rendering normally flows through TypeApiOps.apply, which threads
the cast's resolved
+// zone), so default to the session-local time zone. It is passed by-name and
the parent builds
+// its formatter lazily, so the config is read only if a value is actually
rendered through this
+// server-side ops - never on the physical-type / literal / row-accessor /
serde paths.
case class TimestampLTZNanosTypeOps(override val t: TimestampLTZNanosType)
- extends TimestampLTZNanosTypeApiOps(t, ZoneOffset.UTC) with
TimestampNanosTypeOps {
+ extends TimestampLTZNanosTypeApiOps(t,
DateTimeUtils.getZoneId(SQLConf.get.sessionLocalTimeZone))
+ with TimestampNanosTypeOps {
override def getPhysicalType: PhysicalDataType =
PhysicalTimestampLTZNanosType
diff --git
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TypeOps.scala
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TypeOps.scala
index f11a3bb07156..b09feb0fb1cf 100644
---
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TypeOps.scala
+++
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TypeOps.scala
@@ -27,6 +27,7 @@ import org.apache.spark.sql.catalyst.expressions.{Expression,
Literal, MutableVa
import org.apache.spark.sql.catalyst.types.PhysicalDataType
import org.apache.spark.sql.execution.arrow.ArrowFieldWriter
import org.apache.spark.sql.types.{DataType, TimestampLTZNanosType,
TimestampNTZNanosType, TimeType}
+import org.apache.spark.sql.types.ops.TypeApiOps
/**
* Server-side (catalyst) type operations for the Types Framework.
@@ -36,6 +37,12 @@ import org.apache.spark.sql.types.{DataType,
TimestampLTZNanosType, TimestampNTZ
* implemented by every type. Optional methods (serialization, Arrow writer)
return Option and
* default to None - types implement them as they expand their integration
coverage.
*
+ * Extends [[TypeApiOps]] (the api-side trait) so that callers holding an
`Option[TypeOps]` can
+ * invoke client-side methods (`format`, `toSQLValue`, `getEncoder`, ...)
directly via `.map`,
+ * instead of an `isInstanceOf[TypeApiOps]` runtime narrowing. Concrete
`*TypeOps` classes typically
+ * extend `*TypeApiOps` to inherit api-side method implementations -- this
trait makes that
+ * relationship part of the type system rather than a runtime invariant.
+ *
* USAGE - integration points use TypeOps(dt) which returns Option[TypeOps]:
* {{{
* def getPhysicalType(dt: DataType): PhysicalDataType =
@@ -48,7 +55,8 @@ import org.apache.spark.sql.types.{DataType,
TimestampLTZNanosType, TimestampNTZ
* }}}
*
* IMPLEMENTATION - to add a new type to the framework:
- * 1. Create a case class extending TypeOps (and optionally TypeApiOps for
client-side ops)
+ * 1. Create a case class extending the matching `*TypeApiOps` and mixing in
`TypeOps`
+ * (e.g., `case class FooTypeOps(...) extends FooTypeApiOps(...) with
TypeOps`)
* 2. Register it in TypeOps.apply() below - single registration point
* 3. No other file modifications needed - all integration points
automatically work
*
@@ -56,10 +64,7 @@ import org.apache.spark.sql.types.{DataType,
TimestampLTZNanosType, TimestampNTZ
* TimeTypeOps for a reference implementation
* @since 4.2.0
*/
-trait TypeOps extends Serializable {
-
- /** The DataType this Ops instance handles. */
- def dataType: DataType
+trait TypeOps extends TypeApiOps {
// ==================== Physical Type Representation ====================
diff --git
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/types/ops/TimestampNanosTypeOpsSuite.scala
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/types/ops/TimestampNanosTypeOpsSuite.scala
index 6633416a0645..2cff609c870e 100644
---
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/types/ops/TimestampNanosTypeOpsSuite.scala
+++
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/types/ops/TimestampNanosTypeOpsSuite.scala
@@ -216,6 +216,22 @@ class TimestampNanosTypeOpsSuite extends SparkFunSuite
with SQLHelper {
}
}
+ test("SPARK-57285: catalyst LTZ ops renders in the session-local time zone
(not a fixed UTC)") {
+ // The server-side TimestampLTZNanosTypeOps defaults its render zone to
the session-local time
+ // zone (passed by-name, forced lazily at first render). Use a non-UTC
zone so a regression back
+ // to a fixed UTC zone would change the rendered wall clock and fail here.
+ withSQLConf(SQLConf.SESSION_LOCAL_TIMEZONE.key -> "America/Los_Angeles") {
+ precisions.foreach { p =>
+ val ops = TimestampLTZNanosTypeOps(TimestampLTZNanosType(p))
+ val v =
DateTimeUtils.instantToTimestampNanos(nanosLdt.toInstant(ZoneOffset.UTC), p)
+ val frac = expectedFraction(p)
+ // 2020-01-01 00:00 UTC is 2019-12-31 16:00 in America/Los_Angeles
(UTC-8 in January).
+ assert(ops.format(v) === s"2019-12-31 16:00:00.$frac", s"LTZ
session-zone format for p=$p")
+ assert(ops.toSQLValue(v) === s"TIMESTAMP_LTZ '2019-12-31
16:00:00.$frac'")
+ }
+ }
+ }
+
test("SPARK-57285: NTZ formatExternal renders LocalDateTime at the column
precision") {
precisions.foreach { p =>
val ops = TypeApiOps(TimestampNTZNanosType(p)).get
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
