(spark) branch branch-4.x updated: [MINOR][SQL] Make TypeOps extend TypeApiOps in the Types Framework

[ma...@apache.org]

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 <da...@databricks.com>
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 <da...@databricks.com>
    Signed-off-by: Max Gekk <ma...@gmail.com>
    (cherry picked from commit 64ceedcd43df02462fdc43f7809ad063f3b8b5dc)
    Signed-off-by: Max Gekk <ma...@gmail.com>
---
 .../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: commits-unsubscribe@spark.apache.org
For additional commands, e-mail: commits-help@spark.apache.org