You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "davidm-db (via GitHub)" <gi...@apache.org> on 2026/02/09 09:20:17 UTC
[PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
davidm-db opened a new pull request, #54223:
URL: https://github.com/apache/spark/pull/54223
### What changes were proposed in this pull request?
This PR introduces the foundation of the **Spark Types Framework** - a system for centralizing type-specific operations that are currently scattered across 50+ files using diverse patterns.
**Framework interfaces** (9 new files in `sql/api` and `sql/catalyst`):
- Base traits: `TypeOps` (catalyst), `TypeApiOps` (sql-api)
- Core interfaces: `PhyTypeOps` (physical type representation), `LiteralTypeOps` (literal creation), `ExternalTypeOps` (internal/external type conversion), `FormatTypeOps` (string formatting), `EncodeTypeOps` (row encoders)
- Proof-of-concept implementation: `TimeTypeOps` + `TimeTypeApiOps` for TimeType
**Integration points** (10 existing files modified with check-and-delegate pattern):
- `PhysicalDataType.scala` - physical type dispatch
- `CatalystTypeConverters.scala` - external/internal type conversion (via `TypeOpsConverter` adapter)
- `ToStringBase.scala` - string formatting
- `RowEncoder.scala` - row encoding
- `literals.scala` - default literal creation (`Literal.default`)
- `EncoderUtils.scala` - encoder Java class mapping
- `CodeGenerator.scala` - codegen Java class mapping
- `SpecificInternalRow.scala` - mutable value creation
- `InternalRow.scala` - row writer dispatch
**Feature flag**: `spark.sql.types.framework.enabled` (defaults to `true` in tests via `Utils.isTesting`, `false` otherwise), configured in `SQLConf.scala` + `SqlApiConf.scala`.
**Interface hierarchy:**
```
TypeOps (catalyst)
+-- PhyTypeOps - Physical representation (PhysicalDataType, Java class, MutableValue, row writer)
+-- LiteralTypeOps - Literal creation and defaults
+-- ExternalTypeOps - External <-> internal type conversion
TypeApiOps (sql-api)
+-- FormatTypeOps - String formatting (CAST to STRING, SQL values)
+-- EncodeTypeOps - Row encoding (AgnosticEncoder)
```
The split across `sql/api` and `sql/catalyst` follows existing Spark module separation - `TypeApiOps` lives in `sql/api` for client-side operations that depend on `AgnosticEncoder`, while `TypeOps` lives in `sql/catalyst` for server-side operations that depend on `InternalRow`, `PhysicalDataType`, etc.
All integration points use a **check-and-delegate** pattern at the beginning of existing match statements:
```scala
def someOperation(dt: DataType) = dt match {
// Framework dispatch (guarded by feature flag)
case _ if SQLConf.get.typesFrameworkEnabled && SomeOps.supports(dt) =>
SomeOps(dt).someMethod()
// Legacy types (unchanged)
case DateType => ...
case TimestampType => ...
}
```
This is the first of several planned PRs. Subsequent PRs will add client-side integrations (Spark Connect proto, Arrow SerDe, JDBC, Python, Thrift) and storage format integrations (Parquet, ORC, CSV, JSON, etc.).
### Why are the changes needed?
Adding a new data type to Spark currently requires modifying **50+ files** with scattered type-specific logic. Each file has its own conventions, and there is no compiler assistance to ensure completeness. Integration points are non-obvious and easy to miss - patterns include `_: TimeType` in Scala pattern matching, `TimeNanoVector` in Arrow SerDe, `.hasTime()`/`.getTime()` in proto fields, `LocalTimeEncoder` in encoder helpers, `java.sql.Types.TIME` in JDBC, `instanceof TimeType` in Java files, and compound matches like `case LongType | ... | _: TimeType =>` that are invisible to naive searches.
The framework centralizes type-specific infrastructure operations in Ops interface classes. When adding a new type with the framework in place, a developer creates two Ops classes (one in `sql/api`, one in `sql/catalyst`) and registers them in the corresponding factory objects. The compiler enforces that all required interface methods are implemented, significantly reducing the risk of missing integration points.
**Concrete example - TimeType:** TimeType has integration points spread across 50+ files using the diverse patterns listed above (physical type mapping, literals, type converters, encoders, formatters, Arrow SerDe, proto conversion, JDBC, Python, Thrift, storage formats). With the framework, these are consolidated into two Ops classes: `TimeTypeOps` (~150 lines) and `TimeTypeApiOps` (~90 lines). A developer adding a new type with similar complexity would create two analogous files instead of touching 50+ files. The framework does not cover type-specific expressions (e.g., `CurrentTime`, `TimeAddInterval`) or SQL parser changes, which are inherently type-specific - it provides the primitives those build on.
This PR covers the core infrastructure integration. Subsequent PRs will add client-side integrations (Spark Connect proto, Arrow SerDe, JDBC, Python, Thrift) and storage format integrations (Parquet, ORC, CSV, JSON, etc.).
### Does this PR introduce _any_ user-facing change?
No. This is an internal refactoring behind a feature flag (`spark.sql.types.framework.enabled`). When the flag is enabled, framework-supported types use centralized Ops dispatch instead of direct pattern matching. Behavior is identical in both paths. The flag defaults to `true` in tests and `false` otherwise.
### How was this patch tested?
The framework is a refactoring of existing dispatch logic - it changes the mechanism but preserves identical behavior. The feature flag is enabled by default in test environments (`Utils.isTesting`), so the entire existing test suite validates the framework code path. No new tests are added in this PR because the framework delegates to the same underlying logic that existing tests already cover.
In subsequent phases, the testing focus will be on:
1. Testing the framework itself (Ops interface contracts, roundtrip correctness, edge cases)
2. Designing a generalized testing mechanism that enforces proper test coverage for each type added through the framework
### Was this patch authored or co-authored using generative AI tooling?
Co-authored with: claude-opus-4-6
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2872101793
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TypeApiOps.scala:
##########
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.sql.catalyst.encoders.AgnosticEncoder
+import org.apache.spark.sql.internal.SqlApiConf
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.unsafe.types.UTF8String
+
+/**
+ * Client-side (spark-api) type operations for the Types Framework.
+ *
+ * This trait consolidates all client-side operations that a data type must implement to be usable
+ * in the Spark SQL API layer. All methods are mandatory because a type cannot function correctly
+ * without string formatting (needed for CAST to STRING, EXPLAIN, SHOW) or encoding (needed for
+ * Dataset[T] operations).
+ *
+ * This single-interface design was chosen over separate FormatTypeOps/EncodeTypeOps traits to
+ * make it clear what a new type must implement - there is one mandatory interface, and it
+ * contains everything required. Optional capabilities (e.g., proto, Arrow, JDBC) are defined as
+ * separate traits that can be mixed in incrementally.
+ *
+ * RELATIONSHIP TO TypeOps:
+ * - TypeOps (catalyst): Server-side operations - physical types, literals, conversions
+ * - TypeApiOps (spark-api): Client-side operations - formatting, encoding
+ *
+ * The split exists because sql/api cannot depend on sql/catalyst. For TimeType, TimeTypeOps
+ * (catalyst) extends TimeTypeApiOps (sql-api) to inherit both sets of operations.
+ *
+ * @see
+ * TimeTypeApiOps for reference implementation
+ * @since 4.2.0
+ */
+trait TypeApiOps extends Serializable {
+
+ /** The DataType this Ops instance handles. */
+ def dataType: DataType
+
+ // ==================== String Formatting ====================
+
+ /**
+ * Formats an internal value as a display string.
+ *
+ * Used by CAST to STRING, EXPLAIN output, SHOW commands.
+ *
+ * @param v
+ * the internal value (e.g., Long nanoseconds for TimeType)
+ * @return
+ * formatted string (e.g., "10:30:45.123456")
+ */
+ def format(v: Any): String
+
+ /**
+ * Formats an internal value as a UTF8String.
+ *
+ * Default implementation wraps format(). Override for performance if needed.
+ */
+ def formatUTF8(v: Any): UTF8String = UTF8String.fromString(format(v))
Review Comment:
nvm, I see the use case.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871925040
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala:
##########
@@ -186,42 +187,45 @@ object Literal {
/**
* Create a literal with default value for given DataType
*/
- def default(dataType: DataType): Literal = dataType match {
Review Comment:
done
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimeTypeOps.scala:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.types.ops
+
+import java.time.LocalTime
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.catalyst.expressions.{Literal, MutableLong, MutableValue}
+import org.apache.spark.sql.catalyst.types.{PhysicalDataType, PhysicalLongType}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.sql.types.ops.TimeTypeApiOps
+
+/**
+ * Server-side (catalyst) operations for TimeType.
+ *
+ * This class implements all TypeOps methods for the TIME data type, providing:
+ * - Physical type: PhysicalLongType (nanoseconds since midnight)
+ * - Literals: default is 0L (midnight)
+ * - External conversion: java.time.LocalTime <-> Long nanoseconds
+ *
+ * It also inherits client-side operations from TimeTypeApiOps:
+ * - String formatting (FractionTimeFormatter)
+ * - Row encoding (LocalTimeEncoder)
+ *
+ * INTERNAL REPRESENTATION:
+ * - Values stored as Long nanoseconds since midnight
+ * - Range: 0 to 86,399,999,999,999
+ * - External type: java.time.LocalTime
+ * - Precision (0-6) affects display only, not storage
+ *
+ * @param t
+ * The TimeType with precision information
+ * @since 4.2.0
+ */
+case class TimeTypeOps(override val t: TimeType) extends TimeTypeApiOps(t) with TypeOps {
+
+ override def dataType: DataType = t
Review Comment:
done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871383968
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimeTypeOps.scala:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.types.ops
+
+import java.time.LocalTime
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.catalyst.expressions.{Literal, MutableLong, MutableValue}
+import org.apache.spark.sql.catalyst.types.{PhysicalDataType, PhysicalLongType}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.sql.types.ops.TimeTypeApiOps
+
+/**
+ * Server-side (catalyst) operations for TimeType.
+ *
+ * This class implements all TypeOps methods for the TIME data type, providing:
+ * - Physical type: PhysicalLongType (nanoseconds since midnight)
+ * - Literals: default is 0L (midnight)
+ * - External conversion: java.time.LocalTime <-> Long nanoseconds
+ *
+ * It also inherits client-side operations from TimeTypeApiOps:
+ * - String formatting (FractionTimeFormatter)
+ * - Row encoding (LocalTimeEncoder)
+ *
+ * INTERNAL REPRESENTATION:
+ * - Values stored as Long nanoseconds since midnight
+ * - Range: 0 to 86,399,999,999,999
+ * - External type: java.time.LocalTime
+ * - Precision (0-6) affects display only, not storage
+ *
+ * @param t
+ * The TimeType with precision information
+ * @since 4.2.0
+ */
+case class TimeTypeOps(override val t: TimeType) extends TimeTypeApiOps(t) with TypeOps {
+
+ override def dataType: DataType = t
+
+ // ==================== Physical Type Representation ====================
+
+ override def getPhysicalType: PhysicalDataType = PhysicalLongType
+
+ override def getJavaClass: Class[_] = classOf[Long]
+
+ override def getMutableValue: MutableValue = new MutableLong
+
+ override def getRowWriter(ordinal: Int): (InternalRow, Any) => Unit =
+ (input, v) => input.setLong(ordinal, v.asInstanceOf[Long])
+
+ // ==================== Literal Creation ====================
+
+ override def getDefaultLiteral: Literal = Literal.create(0L, t)
+
+ override def getJavaLiteral(v: Any): String = s"${v}L"
+
+ // ==================== External Type Conversion ====================
+
+ override def toCatalystImpl(scalaValue: Any): Any = {
+ DateTimeUtils.localTimeToNanos(scalaValue.asInstanceOf[LocalTime])
+ }
+
+ override def toScala(catalystValue: Any): Any = {
+ if (catalystValue == null) null
+ else DateTimeUtils.nanosToLocalTime(catalystValue.asInstanceOf[Long])
+ }
+
+ override def toScalaImpl(row: InternalRow, column: Int): Any = {
+ DateTimeUtils.nanosToLocalTime(row.getLong(column))
Review Comment:
shall we check `row.isNullAt`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871943279
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TypeApiOps.scala:
##########
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.sql.catalyst.encoders.AgnosticEncoder
+import org.apache.spark.sql.internal.SqlApiConf
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.unsafe.types.UTF8String
+
+/**
+ * Client-side (spark-api) type operations for the Types Framework.
+ *
+ * This trait consolidates all client-side operations that a data type must implement to be usable
+ * in the Spark SQL API layer. All methods are mandatory because a type cannot function correctly
+ * without string formatting (needed for CAST to STRING, EXPLAIN, SHOW) or encoding (needed for
+ * Dataset[T] operations).
+ *
+ * This single-interface design was chosen over separate FormatTypeOps/EncodeTypeOps traits to
+ * make it clear what a new type must implement - there is one mandatory interface, and it
+ * contains everything required. Optional capabilities (e.g., proto, Arrow, JDBC) are defined as
+ * separate traits that can be mixed in incrementally.
+ *
+ * RELATIONSHIP TO TypeOps:
+ * - TypeOps (catalyst): Server-side operations - physical types, literals, conversions
+ * - TypeApiOps (spark-api): Client-side operations - formatting, encoding
+ *
+ * The split exists because sql/api cannot depend on sql/catalyst. For TimeType, TimeTypeOps
+ * (catalyst) extends TimeTypeApiOps (sql-api) to inherit both sets of operations.
+ *
+ * @see
+ * TimeTypeApiOps for reference implementation
+ * @since 4.2.0
+ */
+trait TypeApiOps extends Serializable {
+
+ /** The DataType this Ops instance handles. */
+ def dataType: DataType
+
+ // ==================== String Formatting ====================
+
+ /**
+ * Formats an internal value as a display string.
+ *
+ * Used by CAST to STRING, EXPLAIN output, SHOW commands.
+ *
+ * @param v
+ * the internal value (e.g., Long nanoseconds for TimeType)
+ * @return
+ * formatted string (e.g., "10:30:45.123456")
+ */
+ def format(v: Any): String
Review Comment:
at the moment, it's only called from the `formatUTF8` below, but in one of the later phases it will be called from `Row.scala` (client/api) and `HiveResult.scala` (core).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871952899
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimeTypeOps.scala:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.types.ops
+
+import java.time.LocalTime
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.catalyst.expressions.{Literal, MutableLong, MutableValue}
+import org.apache.spark.sql.catalyst.types.{PhysicalDataType, PhysicalLongType}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.sql.types.ops.TimeTypeApiOps
+
+/**
+ * Server-side (catalyst) operations for TimeType.
+ *
+ * This class implements all TypeOps methods for the TIME data type, providing:
+ * - Physical type: PhysicalLongType (nanoseconds since midnight)
+ * - Literals: default is 0L (midnight)
+ * - External conversion: java.time.LocalTime <-> Long nanoseconds
+ *
+ * It also inherits client-side operations from TimeTypeApiOps:
+ * - String formatting (FractionTimeFormatter)
+ * - Row encoding (LocalTimeEncoder)
+ *
+ * INTERNAL REPRESENTATION:
+ * - Values stored as Long nanoseconds since midnight
+ * - Range: 0 to 86,399,999,999,999
+ * - External type: java.time.LocalTime
+ * - Precision (0-6) affects display only, not storage
+ *
+ * @param t
+ * The TimeType with precision information
+ * @since 4.2.0
+ */
+case class TimeTypeOps(override val t: TimeType) extends TimeTypeApiOps(t) with TypeOps {
+
+ override def dataType: DataType = t
+
+ // ==================== Physical Type Representation ====================
+
+ override def getPhysicalType: PhysicalDataType = PhysicalLongType
+
+ override def getJavaClass: Class[_] = classOf[Long]
+
+ override def getMutableValue: MutableValue = new MutableLong
+
+ override def getRowWriter(ordinal: Int): (InternalRow, Any) => Unit =
+ (input, v) => input.setLong(ordinal, v.asInstanceOf[Long])
+
+ // ==================== Literal Creation ====================
+
+ override def getDefaultLiteral: Literal = Literal.create(0L, t)
+
+ override def getJavaLiteral(v: Any): String = s"${v}L"
+
+ // ==================== External Type Conversion ====================
+
+ override def toCatalystImpl(scalaValue: Any): Any = {
+ DateTimeUtils.localTimeToNanos(scalaValue.asInstanceOf[LocalTime])
+ }
+
+ override def toScala(catalystValue: Any): Any = {
+ if (catalystValue == null) null
+ else DateTimeUtils.nanosToLocalTime(catalystValue.asInstanceOf[Long])
+ }
+
+ override def toScalaImpl(row: InternalRow, column: Int): Any = {
+ DateTimeUtils.nanosToLocalTime(row.getLong(column))
Review Comment:
if it's fine by you, I'd like to check this as a follow-up. it's TIME specific logic, probably there since Max added it while supporting TIME datatype. the framework just retains the original behavior and I think it's better to address this in a separate PR since it's not related to the framework and I'm not sure what the implications might be from the top of my mind...
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871375213
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/EncoderUtils.scala:
##########
@@ -98,19 +99,21 @@ object EncoderUtils {
}
def dataTypeJavaClass(dt: DataType): Class[_] = {
- dt match {
- case _: DecimalType => classOf[Decimal]
- case _: DayTimeIntervalType => classOf[PhysicalLongType.InternalType]
- case _: YearMonthIntervalType => classOf[PhysicalIntegerType.InternalType]
- case _: TimeType => classOf[PhysicalLongType.InternalType]
- case _: StringType => classOf[UTF8String]
- case _: StructType => classOf[InternalRow]
- case _: ArrayType => classOf[ArrayData]
- case _: MapType => classOf[MapData]
- case _: GeographyType => classOf[GeographyVal]
- case _: GeometryType => classOf[GeometryVal]
- case ObjectType(cls) => cls
- case _ => typeJavaMapping.getOrElse(dt, classOf[java.lang.Object])
+ TypeOps(dt).map(_.getJavaClass).getOrElse {
Review Comment:
ditto for a different code style to reduce diff
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871392284
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TypeApiOps.scala:
##########
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.sql.catalyst.encoders.AgnosticEncoder
+import org.apache.spark.sql.internal.SqlApiConf
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.unsafe.types.UTF8String
+
+/**
+ * Client-side (spark-api) type operations for the Types Framework.
+ *
+ * This trait consolidates all client-side operations that a data type must implement to be usable
+ * in the Spark SQL API layer. All methods are mandatory because a type cannot function correctly
+ * without string formatting (needed for CAST to STRING, EXPLAIN, SHOW) or encoding (needed for
+ * Dataset[T] operations).
+ *
+ * This single-interface design was chosen over separate FormatTypeOps/EncodeTypeOps traits to
+ * make it clear what a new type must implement - there is one mandatory interface, and it
+ * contains everything required. Optional capabilities (e.g., proto, Arrow, JDBC) are defined as
+ * separate traits that can be mixed in incrementally.
+ *
+ * RELATIONSHIP TO TypeOps:
+ * - TypeOps (catalyst): Server-side operations - physical types, literals, conversions
+ * - TypeApiOps (spark-api): Client-side operations - formatting, encoding
+ *
+ * The split exists because sql/api cannot depend on sql/catalyst. For TimeType, TimeTypeOps
+ * (catalyst) extends TimeTypeApiOps (sql-api) to inherit both sets of operations.
+ *
+ * @see
+ * TimeTypeApiOps for reference implementation
+ * @since 4.2.0
+ */
+trait TypeApiOps extends Serializable {
+
+ /** The DataType this Ops instance handles. */
+ def dataType: DataType
+
+ // ==================== String Formatting ====================
+
+ /**
+ * Formats an internal value as a display string.
+ *
+ * Used by CAST to STRING, EXPLAIN output, SHOW commands.
+ *
+ * @param v
+ * the internal value (e.g., Long nanoseconds for TimeType)
+ * @return
+ * formatted string (e.g., "10:30:45.123456")
+ */
+ def format(v: Any): String
Review Comment:
where do we call it at the client side?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2853249854
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/EncodeTypeOps.scala:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.sql.catalyst.encoders.AgnosticEncoder
+import org.apache.spark.sql.types.DataType
+
+/**
+ * Operations for row encoding and decoding.
+ *
+ * PURPOSE: Provides the encoder needed for Dataset[T] operations. The encoder handles
+ * serialization and deserialization of user objects to/from internal rows.
+ *
+ * USAGE CONTEXT - examples of use:
+ * - RowEncoder.scala - creates encoders for schema fields
+ * - EncoderUtils.scala - encoder utility functions
+ * - Spark Connect - client-side encoding
+ * - Dataset[T] API - all typed dataset operations
+ *
+ * @see
+ * TimeTypeApiOps for reference implementation
+ * @since 4.1.0
+ */
+trait EncodeTypeOps extends TypeApiOps {
Review Comment:
My worry is too many type ops interfaces may become another challenge for adding new data types. It's unclear what ops to implement and which ones are optional.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2872096751
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TypeApiOps.scala:
##########
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.sql.catalyst.encoders.AgnosticEncoder
+import org.apache.spark.sql.internal.SqlApiConf
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.unsafe.types.UTF8String
+
+/**
+ * Client-side (spark-api) type operations for the Types Framework.
+ *
+ * This trait consolidates all client-side operations that a data type must implement to be usable
+ * in the Spark SQL API layer. All methods are mandatory because a type cannot function correctly
+ * without string formatting (needed for CAST to STRING, EXPLAIN, SHOW) or encoding (needed for
+ * Dataset[T] operations).
+ *
+ * This single-interface design was chosen over separate FormatTypeOps/EncodeTypeOps traits to
+ * make it clear what a new type must implement - there is one mandatory interface, and it
+ * contains everything required. Optional capabilities (e.g., proto, Arrow, JDBC) are defined as
+ * separate traits that can be mixed in incrementally.
+ *
+ * RELATIONSHIP TO TypeOps:
+ * - TypeOps (catalyst): Server-side operations - physical types, literals, conversions
+ * - TypeApiOps (spark-api): Client-side operations - formatting, encoding
+ *
+ * The split exists because sql/api cannot depend on sql/catalyst. For TimeType, TimeTypeOps
+ * (catalyst) extends TimeTypeApiOps (sql-api) to inherit both sets of operations.
+ *
+ * @see
+ * TimeTypeApiOps for reference implementation
+ * @since 4.2.0
+ */
+trait TypeApiOps extends Serializable {
+
+ /** The DataType this Ops instance handles. */
+ def dataType: DataType
+
+ // ==================== String Formatting ====================
+
+ /**
+ * Formats an internal value as a display string.
+ *
+ * Used by CAST to STRING, EXPLAIN output, SHOW commands.
+ *
+ * @param v
+ * the internal value (e.g., Long nanoseconds for TimeType)
+ * @return
+ * formatted string (e.g., "10:30:45.123456")
+ */
+ def format(v: Any): String
+
+ /**
+ * Formats an internal value as a UTF8String.
+ *
+ * Default implementation wraps format(). Override for performance if needed.
+ */
+ def formatUTF8(v: Any): UTF8String = UTF8String.fromString(format(v))
Review Comment:
do we expect any data type to override it? It seems weird if `format` and `formatUTF8` are inconsistent. We can probably remove this API.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871376317
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -1989,30 +1990,32 @@ object CodeGenerator extends Logging {
}
}
- @tailrec
- def javaClass(dt: DataType): Class[_] = dt match {
- case BooleanType => java.lang.Boolean.TYPE
- case ByteType => java.lang.Byte.TYPE
- case ShortType => java.lang.Short.TYPE
- case IntegerType | DateType | _: YearMonthIntervalType => java.lang.Integer.TYPE
- case LongType | TimestampType | TimestampNTZType | _: DayTimeIntervalType | _: TimeType =>
- java.lang.Long.TYPE
- case FloatType => java.lang.Float.TYPE
- case DoubleType => java.lang.Double.TYPE
- case _: DecimalType => classOf[Decimal]
- case BinaryType => classOf[Array[Byte]]
- case _: GeographyType => classOf[GeographyVal]
- case _: GeometryType => classOf[GeometryVal]
- case _: StringType => classOf[UTF8String]
- case CalendarIntervalType => classOf[CalendarInterval]
- case _: StructType => classOf[InternalRow]
- case _: ArrayType => classOf[ArrayData]
- case _: MapType => classOf[MapData]
- case udt: UserDefinedType[_] => javaClass(udt.sqlType)
- case ObjectType(cls) => cls
- case VariantType => classOf[VariantVal]
- case _ => classOf[Object]
- }
+ def javaClass(dt: DataType): Class[_] =
+ TypeOps(dt).map(_.getJavaClass).getOrElse {
Review Comment:
ditto
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala:
##########
@@ -186,42 +187,45 @@ object Literal {
/**
* Create a literal with default value for given DataType
*/
- def default(dataType: DataType): Literal = dataType match {
Review Comment:
ditto
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2852005145
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/EncodeTypeOps.scala:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.sql.catalyst.encoders.AgnosticEncoder
+import org.apache.spark.sql.types.DataType
+
+/**
+ * Operations for row encoding and decoding.
+ *
+ * PURPOSE: Provides the encoder needed for Dataset[T] operations. The encoder handles
+ * serialization and deserialization of user objects to/from internal rows.
+ *
+ * USAGE CONTEXT - examples of use:
+ * - RowEncoder.scala - creates encoders for schema fields
+ * - EncoderUtils.scala - encoder utility functions
+ * - Spark Connect - client-side encoding
+ * - Dataset[T] API - all typed dataset operations
+ *
+ * @see
+ * TimeTypeApiOps for reference implementation
+ * @since 4.1.0
+ */
+trait EncodeTypeOps extends TypeApiOps {
Review Comment:
if every type should implement it, shall we merge it into the base trait `TypeApiOps`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2853018332
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TypeApiOps.scala:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.SparkException
+import org.apache.spark.sql.types.{DataType, TimeType}
+
+/**
+ * Base trait for client-side (spark-api) type operations.
+ *
+ * PURPOSE: TypeApiOps handles operations that require spark-api internals (e.g., AgnosticEncoder)
+ * that are not available in the catalyst package. This separation prevents circular dependencies
+ * between sql/api and sql/catalyst modules.
+ *
+ * USAGE - examples of use:
+ * - Row encoding/decoding (EncodeTypeOps)
+ * - String formatting (FormatTypeOps)
+ *
+ * RELATIONSHIP TO TypeOps:
+ * - TypeOps (catalyst): Server-side operations - physical types, literals, conversions
+ * - TypeApiOps (spark-api): Client-side operations - encoding, formatting
+ *
+ * For TimeType, TimeTypeOps extends TimeTypeApiOps to inherit both sets of operations.
+ *
+ * @see
+ * TimeTypeApiOps for a reference implementation
+ * @since 4.1.0
+ */
+trait TypeApiOps extends Serializable {
+
+ /** The DataType this Ops instance handles */
+ def dataType: DataType
+}
+
+/**
+ * Factory object for creating TypeApiOps instances.
+ */
+object TypeApiOps {
+
+ /**
+ * Creates a TypeApiOps instance for the given DataType.
+ *
+ * @param dt
+ * The DataType to get operations for
+ * @return
+ * TypeApiOps instance for the type
+ * @throws SparkException
+ * if no TypeApiOps implementation exists for the type
+ */
+ def apply(dt: DataType): TypeApiOps = dt match {
+ case tt: TimeType => new TimeTypeApiOps(tt)
+ // Future types will be added here
+ case _ =>
+ throw SparkException.internalError(
+ s"No TypeApiOps implementation for ${dt.typeName}. " +
+ "This type is not yet supported by the Types Framework.")
+ }
+
+ /**
+ * Checks if a DataType is supported by the Types Framework (client-side).
+ *
+ * @param dt
+ * The DataType to check
+ * @return
+ * true if the type is supported by the framework
+ */
+ def supports(dt: DataType): Boolean = dt match {
+ case _: TimeType => true
Review Comment:
Sorry for a long message...
I'm honestly not exactly sure on what is the best approach here and I think there are a couple of options that have different tradeoffs. We still need to have semantics of `supports` and `apply`, because that is the pattern that we need in a bunch of places throughout the code, e.g:
```
case _ if *TypeOps.supports(dt) => *TypeOps(dt).<func>()
```
We need this pattern for two reasons:
- not all types will go through the framework, only new ones - we need the `supports` semantics
- we want to allow for incremental implementation of Ops interfaces, or in some specific cases that cannot be handled by the current state of framework, allow the logic to be implemented outside of the framework
Now, we could make this cleaner by doing something like this to enlist into a single place:
```
object TypeApiOps {
def get(dt: DataType): Option[TypeApiOps] = dt match {
case tt: TimeType => Some(TimeTypeApiOps(tt))
// single place to register
case _ => None
}
def apply(dt: DataType): TypeApiOps = get(dt).getOrElse(
throw SparkException.internalError(...))
def supports(dt: DataType): Boolean = get(dt).isDefined
}
```
I see three options (trading off between the ease of maintenance and efficiency):
- Current design (each `Ops` class delgates to `TypeOps`), for example:
- `PhyTypeOps.supports(dt)` -> `TypeOps.supports(dt)` -> 1 pattern match
- `PhyTypeOps(dt)` -> `TypeOps(dt).asInstanceOf[PhyTypeOps]` -> 1 pattern match + 1 allocation + 1 cast
- Total: 2 matches, 1 allocation, 1 cast
- Proposed design with `get()`:
- `PhyTypeOps.supports(dt)` -> `TypeOps.get(dt).collect{...}.isDefined` -> 1 match + Some + collect/Option
- `PhyTypeOps(dt)` -> `TypeOps.get(dt).collect{...}.getOrElse(...)` → same thing again
- Total: 2 matches, ~4 allocations
- Most efficient solution is to have the current design, but each of them implements its own `supports` and `apply`:
- `PhyTypeOps.supports(dt)` -> direct `case _: TimeType => true` -> 1 pattern match
- `PhyTypeOps(dt)` -> direct `case tt: TimeType => TimeTypeOps(tt)` -> 1 pattern match + 1 allocation
- Total: 2 matches, 1 allocation. No delegation, no Options, no casts.
Also, if we want to support incremental implementation, we might want to either:
- Let each Ops interface implement its own `supports` - overhead of enlisting the type in each Ops class.
- Do something like:
```
object PhyTypeOps {
def supports(dt: DataType): Boolean = TypeOps.supports(dt) && TypeOps(dt).isInstanceOf[PhyTypeOps]
def apply(dt: DataType): PhyTypeOps = TypeOps(dt).asInstanceOf[PhyTypeOps]
}
```
I'm honestly not sure how important this overhead might be. I think your input here might be really useful.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871952899
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimeTypeOps.scala:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.types.ops
+
+import java.time.LocalTime
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.catalyst.expressions.{Literal, MutableLong, MutableValue}
+import org.apache.spark.sql.catalyst.types.{PhysicalDataType, PhysicalLongType}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.sql.types.ops.TimeTypeApiOps
+
+/**
+ * Server-side (catalyst) operations for TimeType.
+ *
+ * This class implements all TypeOps methods for the TIME data type, providing:
+ * - Physical type: PhysicalLongType (nanoseconds since midnight)
+ * - Literals: default is 0L (midnight)
+ * - External conversion: java.time.LocalTime <-> Long nanoseconds
+ *
+ * It also inherits client-side operations from TimeTypeApiOps:
+ * - String formatting (FractionTimeFormatter)
+ * - Row encoding (LocalTimeEncoder)
+ *
+ * INTERNAL REPRESENTATION:
+ * - Values stored as Long nanoseconds since midnight
+ * - Range: 0 to 86,399,999,999,999
+ * - External type: java.time.LocalTime
+ * - Precision (0-6) affects display only, not storage
+ *
+ * @param t
+ * The TimeType with precision information
+ * @since 4.2.0
+ */
+case class TimeTypeOps(override val t: TimeType) extends TimeTypeApiOps(t) with TypeOps {
+
+ override def dataType: DataType = t
+
+ // ==================== Physical Type Representation ====================
+
+ override def getPhysicalType: PhysicalDataType = PhysicalLongType
+
+ override def getJavaClass: Class[_] = classOf[Long]
+
+ override def getMutableValue: MutableValue = new MutableLong
+
+ override def getRowWriter(ordinal: Int): (InternalRow, Any) => Unit =
+ (input, v) => input.setLong(ordinal, v.asInstanceOf[Long])
+
+ // ==================== Literal Creation ====================
+
+ override def getDefaultLiteral: Literal = Literal.create(0L, t)
+
+ override def getJavaLiteral(v: Any): String = s"${v}L"
+
+ // ==================== External Type Conversion ====================
+
+ override def toCatalystImpl(scalaValue: Any): Any = {
+ DateTimeUtils.localTimeToNanos(scalaValue.asInstanceOf[LocalTime])
+ }
+
+ override def toScala(catalystValue: Any): Any = {
+ if (catalystValue == null) null
+ else DateTimeUtils.nanosToLocalTime(catalystValue.asInstanceOf[Long])
+ }
+
+ override def toScalaImpl(row: InternalRow, column: Int): Any = {
+ DateTimeUtils.nanosToLocalTime(row.getLong(column))
Review Comment:
if it's fine by you, I'd like to check this as a follow-up. it's TIME specific logic, probably there since Max added it while supporting TIME datatype. the framework just retains the original behavior and I think it's better to address this in a separate PR since it's not related to the framework and I'm not sure what the implications might be...
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #54223:
URL: https://github.com/apache/spark/pull/54223#issuecomment-3986532727
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #54223: [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation
URL: https://github.com/apache/spark/pull/54223
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2853018332
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TypeApiOps.scala:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.SparkException
+import org.apache.spark.sql.types.{DataType, TimeType}
+
+/**
+ * Base trait for client-side (spark-api) type operations.
+ *
+ * PURPOSE: TypeApiOps handles operations that require spark-api internals (e.g., AgnosticEncoder)
+ * that are not available in the catalyst package. This separation prevents circular dependencies
+ * between sql/api and sql/catalyst modules.
+ *
+ * USAGE - examples of use:
+ * - Row encoding/decoding (EncodeTypeOps)
+ * - String formatting (FormatTypeOps)
+ *
+ * RELATIONSHIP TO TypeOps:
+ * - TypeOps (catalyst): Server-side operations - physical types, literals, conversions
+ * - TypeApiOps (spark-api): Client-side operations - encoding, formatting
+ *
+ * For TimeType, TimeTypeOps extends TimeTypeApiOps to inherit both sets of operations.
+ *
+ * @see
+ * TimeTypeApiOps for a reference implementation
+ * @since 4.1.0
+ */
+trait TypeApiOps extends Serializable {
+
+ /** The DataType this Ops instance handles */
+ def dataType: DataType
+}
+
+/**
+ * Factory object for creating TypeApiOps instances.
+ */
+object TypeApiOps {
+
+ /**
+ * Creates a TypeApiOps instance for the given DataType.
+ *
+ * @param dt
+ * The DataType to get operations for
+ * @return
+ * TypeApiOps instance for the type
+ * @throws SparkException
+ * if no TypeApiOps implementation exists for the type
+ */
+ def apply(dt: DataType): TypeApiOps = dt match {
+ case tt: TimeType => new TimeTypeApiOps(tt)
+ // Future types will be added here
+ case _ =>
+ throw SparkException.internalError(
+ s"No TypeApiOps implementation for ${dt.typeName}. " +
+ "This type is not yet supported by the Types Framework.")
+ }
+
+ /**
+ * Checks if a DataType is supported by the Types Framework (client-side).
+ *
+ * @param dt
+ * The DataType to check
+ * @return
+ * true if the type is supported by the framework
+ */
+ def supports(dt: DataType): Boolean = dt match {
+ case _: TimeType => true
Review Comment:
Sorry for a long message...
I'm honestly not exactly sure on what is the best approach here and I think there are a couple of options that have different tradeoffs. We still need to have semantics of `supports` and `apply`, because that is the pattern that we need in a bunch of places throughout the code, e.g:
```
case _ if *TypeOps.supports(dt) => *TypeOps(dt).<func>()
```
We need this pattern for two reasons:
- not all types will go through the framework, only new ones - we need the `supports` semantics
- we want to allow for incremental implementation of Ops interfaces, or in some specific cases that cannot be handled by the current state of framework, allow the logic to be implemented outside of the framework
Now, we could make this cleaner by doing something like this to enlist into a single place:
```
object TypeApiOps {
def get(dt: DataType): Option[TypeApiOps] = dt match {
case tt: TimeType => Some(TimeTypeApiOps(tt))
// single place to register
case _ => None
}
def apply(dt: DataType): TypeApiOps = get(dt).getOrElse(
throw SparkException.internalError(...))
def supports(dt: DataType): Boolean = get(dt).isDefined
}
```
I see three options (trading off between the ease of maintenance and efficiency):
- Current design (each `Ops` class delgates to `TypeOps`), for example:
- `PhyTypeOps.supports(dt)` -> `TypeOps.supports(dt)` -> 1 pattern match
- `PhyTypeOps(dt)` -> `TypeOps(dt).asInstanceOf[PhyTypeOps]` -> 1 pattern match + 1 allocation + 1 cast
- Total: 2 matches, 1 allocation, 1 cast
- Proposed design with `get()`:
- `PhyTypeOps.supports(dt)` -> `TypeOps.get(dt).collect{...}.isDefined` -> 1 match + Some + collect/Option
- `PhyTypeOps(dt)` -> `TypeOps.get(dt).collect{...}.getOrElse(...)` → same thing again
- Total: 2 matches, ~4 allocations
- Most efficient solution is to have the current design, but each of them implements its own `supports` and `apply`:
- `PhyTypeOps.supports(dt)` -> direct `case _: TimeType => true` -> 1 pattern match
- `PhyTypeOps(dt)` -> direct `case tt: TimeType => TimeTypeOps(tt)` -> 1 pattern match + 1 allocation
- Total: 2 matches, 1 allocation. No delegation, no Options, no casts.
- This option would render `TypeApiOps` and `TypeOps` as completely not needed.
Also, if we want to support incremental implementation, we might want to either:
- Let each Ops interface implement its own `supports` - overhead of enlisting the type in each Ops class.
- Do something like:
```
object PhyTypeOps {
def supports(dt: DataType): Boolean = TypeOps.supports(dt) && TypeOps(dt).isInstanceOf[PhyTypeOps]
def apply(dt: DataType): PhyTypeOps = TypeOps(dt).asInstanceOf[PhyTypeOps]
}
```
I'm honestly not sure how important this overhead might be. I think your input here might be really useful.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2853067030
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/EncodeTypeOps.scala:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.sql.catalyst.encoders.AgnosticEncoder
+import org.apache.spark.sql.types.DataType
+
+/**
+ * Operations for row encoding and decoding.
+ *
+ * PURPOSE: Provides the encoder needed for Dataset[T] operations. The encoder handles
+ * serialization and deserialization of user objects to/from internal rows.
+ *
+ * USAGE CONTEXT - examples of use:
+ * - RowEncoder.scala - creates encoders for schema fields
+ * - EncoderUtils.scala - encoder utility functions
+ * - Spark Connect - client-side encoding
+ * - Dataset[T] API - all typed dataset operations
+ *
+ * @see
+ * TimeTypeApiOps for reference implementation
+ * @since 4.1.0
+ */
+trait EncodeTypeOps extends TypeApiOps {
Review Comment:
If this is only concerning ApiOps, I think this might make sense. The idea to separate originates from catalyst's `types/ops` where we anticipate much more logic to be added in the future. Here, at the moment I'm not sure we will need much more logic, so I think you're right. If it turns out in the future that the logic is getting more complex, we can decide to separate into multiple Ops interfaces (as it is at the moment).
Was this your thinking? Does it make sense?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2853240095
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TypeApiOps.scala:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.SparkException
+import org.apache.spark.sql.types.{DataType, TimeType}
+
+/**
+ * Base trait for client-side (spark-api) type operations.
+ *
+ * PURPOSE: TypeApiOps handles operations that require spark-api internals (e.g., AgnosticEncoder)
+ * that are not available in the catalyst package. This separation prevents circular dependencies
+ * between sql/api and sql/catalyst modules.
+ *
+ * USAGE - examples of use:
+ * - Row encoding/decoding (EncodeTypeOps)
+ * - String formatting (FormatTypeOps)
+ *
+ * RELATIONSHIP TO TypeOps:
+ * - TypeOps (catalyst): Server-side operations - physical types, literals, conversions
+ * - TypeApiOps (spark-api): Client-side operations - encoding, formatting
+ *
+ * For TimeType, TimeTypeOps extends TimeTypeApiOps to inherit both sets of operations.
+ *
+ * @see
+ * TimeTypeApiOps for a reference implementation
+ * @since 4.1.0
+ */
+trait TypeApiOps extends Serializable {
+
+ /** The DataType this Ops instance handles */
+ def dataType: DataType
+}
+
+/**
+ * Factory object for creating TypeApiOps instances.
+ */
+object TypeApiOps {
+
+ /**
+ * Creates a TypeApiOps instance for the given DataType.
+ *
+ * @param dt
+ * The DataType to get operations for
+ * @return
+ * TypeApiOps instance for the type
+ * @throws SparkException
+ * if no TypeApiOps implementation exists for the type
+ */
+ def apply(dt: DataType): TypeApiOps = dt match {
+ case tt: TimeType => new TimeTypeApiOps(tt)
+ // Future types will be added here
+ case _ =>
+ throw SparkException.internalError(
+ s"No TypeApiOps implementation for ${dt.typeName}. " +
+ "This type is not yet supported by the Types Framework.")
+ }
+
+ /**
+ * Checks if a DataType is supported by the Types Framework (client-side).
+ *
+ * @param dt
+ * The DataType to check
+ * @return
+ * true if the type is supported by the framework
+ */
+ def supports(dt: DataType): Boolean = dt match {
+ case _: TimeType => true
Review Comment:
the caller side is always
```
dt match {
case _ if *TypeOps.supports(dt) => *TypeOps(dt).<func>()
case other types...
}
```
We can change it to
```
*TypeOps(dt).getOrElse {
case other types...
}
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r3078241251
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimeTypeOps.scala:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.types.ops
+
+import java.time.LocalTime
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.catalyst.expressions.{Literal, MutableLong, MutableValue}
+import org.apache.spark.sql.catalyst.types.{PhysicalDataType, PhysicalLongType}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.sql.types.ops.TimeTypeApiOps
+
+/**
+ * Server-side (catalyst) operations for TimeType.
+ *
+ * This class implements all TypeOps methods for the TIME data type, providing:
+ * - Physical type: PhysicalLongType (nanoseconds since midnight)
+ * - Literals: default is 0L (midnight)
+ * - External conversion: java.time.LocalTime <-> Long nanoseconds
+ *
+ * It also inherits client-side operations from TimeTypeApiOps:
+ * - String formatting (FractionTimeFormatter)
+ * - Row encoding (LocalTimeEncoder)
+ *
+ * INTERNAL REPRESENTATION:
+ * - Values stored as Long nanoseconds since midnight
+ * - Range: 0 to 86,399,999,999,999
+ * - External type: java.time.LocalTime
+ * - Precision (0-6) affects display only, not storage
+ *
+ * @param t
+ * The TimeType with precision information
+ * @since 4.2.0
+ */
+case class TimeTypeOps(override val t: TimeType) extends TimeTypeApiOps(t) with TypeOps {
+
+ override def dataType: DataType = t
+
+ // ==================== Physical Type Representation ====================
+
+ override def getPhysicalType: PhysicalDataType = PhysicalLongType
+
+ override def getJavaClass: Class[_] = classOf[Long]
+
+ override def getMutableValue: MutableValue = new MutableLong
+
+ override def getRowWriter(ordinal: Int): (InternalRow, Any) => Unit =
+ (input, v) => input.setLong(ordinal, v.asInstanceOf[Long])
+
+ // ==================== Literal Creation ====================
+
+ override def getDefaultLiteral: Literal = Literal.create(0L, t)
+
+ override def getJavaLiteral(v: Any): String = s"${v}L"
+
+ // ==================== External Type Conversion ====================
+
+ override def toCatalystImpl(scalaValue: Any): Any = {
+ DateTimeUtils.localTimeToNanos(scalaValue.asInstanceOf[LocalTime])
+ }
+
+ override def toScala(catalystValue: Any): Any = {
+ if (catalystValue == null) null
+ else DateTimeUtils.nanosToLocalTime(catalystValue.asInstanceOf[Long])
+ }
+
+ override def toScalaImpl(row: InternalRow, column: Int): Any = {
+ DateTimeUtils.nanosToLocalTime(row.getLong(column))
Review Comment:
The check is not needed because the method is called for non-null columns only. See:
https://github.com/apache/spark/blob/aff735caa3d9c1d26b3941ae961509fa4b95edc5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystTypeConverters.scala#L149-L151
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2852013729
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TypeApiOps.scala:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.SparkException
+import org.apache.spark.sql.types.{DataType, TimeType}
+
+/**
+ * Base trait for client-side (spark-api) type operations.
+ *
+ * PURPOSE: TypeApiOps handles operations that require spark-api internals (e.g., AgnosticEncoder)
+ * that are not available in the catalyst package. This separation prevents circular dependencies
+ * between sql/api and sql/catalyst modules.
+ *
+ * USAGE - examples of use:
+ * - Row encoding/decoding (EncodeTypeOps)
+ * - String formatting (FormatTypeOps)
+ *
+ * RELATIONSHIP TO TypeOps:
+ * - TypeOps (catalyst): Server-side operations - physical types, literals, conversions
+ * - TypeApiOps (spark-api): Client-side operations - encoding, formatting
+ *
+ * For TimeType, TimeTypeOps extends TimeTypeApiOps to inherit both sets of operations.
+ *
+ * @see
+ * TimeTypeApiOps for a reference implementation
+ * @since 4.1.0
+ */
+trait TypeApiOps extends Serializable {
+
+ /** The DataType this Ops instance handles */
+ def dataType: DataType
+}
+
+/**
+ * Factory object for creating TypeApiOps instances.
+ */
+object TypeApiOps {
+
+ /**
+ * Creates a TypeApiOps instance for the given DataType.
+ *
+ * @param dt
+ * The DataType to get operations for
+ * @return
+ * TypeApiOps instance for the type
+ * @throws SparkException
+ * if no TypeApiOps implementation exists for the type
+ */
+ def apply(dt: DataType): TypeApiOps = dt match {
+ case tt: TimeType => new TimeTypeApiOps(tt)
+ // Future types will be added here
+ case _ =>
+ throw SparkException.internalError(
+ s"No TypeApiOps implementation for ${dt.typeName}. " +
+ "This type is not yet supported by the Types Framework.")
+ }
+
+ /**
+ * Checks if a DataType is supported by the Types Framework (client-side).
+ *
+ * @param dt
+ * The DataType to check
+ * @return
+ * true if the type is supported by the framework
+ */
+ def supports(dt: DataType): Boolean = dt match {
+ case _: TimeType => true
Review Comment:
This will duplicate with `def apply` a lot. Shall we make `def apply` return `Option`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2852006200
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/FormatTypeOps.scala:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.sql.types.DataType
+import org.apache.spark.unsafe.types.UTF8String
+
+/**
+ * Operations for formatting values as strings.
+ *
+ * PURPOSE: Handles string formatting for display and SQL output. This includes formatting values
+ * for CAST to STRING, EXPLAIN output, SHOW commands, and SQL literals.
+ *
+ * USAGE CONTEXT - examples of use:
+ * - ToStringBase.scala - CAST(x AS STRING) expressions
+ * - EXPLAIN output - displaying literal values
+ * - SHOW commands - displaying column values
+ * - SQL generation - creating SQL literal representations
+ * - HiveResult.scala - formatting results for Thrift/JDBC
+ *
+ * @see
+ * TimeTypeApiOps for reference implementation
+ * @since 4.1.0
+ */
+trait FormatTypeOps extends TypeApiOps {
+
+ /**
+ * Formats an internal value as a display string.
+ *
+ * This method converts the internal Catalyst representation to a human-readable string suitable
+ * for display or CAST to STRING operations.
+ *
+ * @param v
+ * The internal value (e.g., Long nanoseconds for TimeType)
+ * @return
+ * Formatted string (e.g., "10:30:45.123456")
+ * @example
+ * 37800000000000L -> "10:30:00" (for TIME)
+ */
+ def format(v: Any): String
Review Comment:
ditto
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2852025929
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -71,6 +72,9 @@ object RowEncoder extends DataTypeErrorsBase {
private[sql] def encoderForDataType(dataType: DataType, lenient: Boolean): AgnosticEncoder[_] =
dataType match {
+ // Types Framework: delegate to EncodeTypeOps for supported types when enabled
+ case _ if SqlApiConf.get.typesFrameworkEnabled && EncodeTypeOps.supports(dataType) =>
Review Comment:
shall we check the conf in the narrow wrist `TypeApiOps.apply`, instead of caller sides?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871923190
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -70,62 +71,63 @@ object RowEncoder extends DataTypeErrorsBase {
}
private[sql] def encoderForDataType(dataType: DataType, lenient: Boolean): AgnosticEncoder[_] =
Review Comment:
done
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/EncoderUtils.scala:
##########
@@ -98,19 +99,21 @@ object EncoderUtils {
}
def dataTypeJavaClass(dt: DataType): Class[_] = {
- dt match {
- case _: DecimalType => classOf[Decimal]
- case _: DayTimeIntervalType => classOf[PhysicalLongType.InternalType]
- case _: YearMonthIntervalType => classOf[PhysicalIntegerType.InternalType]
- case _: TimeType => classOf[PhysicalLongType.InternalType]
- case _: StringType => classOf[UTF8String]
- case _: StructType => classOf[InternalRow]
- case _: ArrayType => classOf[ArrayData]
- case _: MapType => classOf[MapData]
- case _: GeographyType => classOf[GeographyVal]
- case _: GeometryType => classOf[GeometryVal]
- case ObjectType(cls) => cls
- case _ => typeJavaMapping.getOrElse(dt, classOf[java.lang.Object])
+ TypeOps(dt).map(_.getJavaClass).getOrElse {
Review Comment:
done
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -1989,30 +1990,32 @@ object CodeGenerator extends Logging {
}
}
- @tailrec
- def javaClass(dt: DataType): Class[_] = dt match {
- case BooleanType => java.lang.Boolean.TYPE
- case ByteType => java.lang.Byte.TYPE
- case ShortType => java.lang.Short.TYPE
- case IntegerType | DateType | _: YearMonthIntervalType => java.lang.Integer.TYPE
- case LongType | TimestampType | TimestampNTZType | _: DayTimeIntervalType | _: TimeType =>
- java.lang.Long.TYPE
- case FloatType => java.lang.Float.TYPE
- case DoubleType => java.lang.Double.TYPE
- case _: DecimalType => classOf[Decimal]
- case BinaryType => classOf[Array[Byte]]
- case _: GeographyType => classOf[GeographyVal]
- case _: GeometryType => classOf[GeometryVal]
- case _: StringType => classOf[UTF8String]
- case CalendarIntervalType => classOf[CalendarInterval]
- case _: StructType => classOf[InternalRow]
- case _: ArrayType => classOf[ArrayData]
- case _: MapType => classOf[MapData]
- case udt: UserDefinedType[_] => javaClass(udt.sqlType)
- case ObjectType(cls) => cls
- case VariantType => classOf[VariantVal]
- case _ => classOf[Object]
- }
+ def javaClass(dt: DataType): Class[_] =
+ TypeOps(dt).map(_.getJavaClass).getOrElse {
Review Comment:
done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r3078241251
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimeTypeOps.scala:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.types.ops
+
+import java.time.LocalTime
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.catalyst.expressions.{Literal, MutableLong, MutableValue}
+import org.apache.spark.sql.catalyst.types.{PhysicalDataType, PhysicalLongType}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.sql.types.ops.TimeTypeApiOps
+
+/**
+ * Server-side (catalyst) operations for TimeType.
+ *
+ * This class implements all TypeOps methods for the TIME data type, providing:
+ * - Physical type: PhysicalLongType (nanoseconds since midnight)
+ * - Literals: default is 0L (midnight)
+ * - External conversion: java.time.LocalTime <-> Long nanoseconds
+ *
+ * It also inherits client-side operations from TimeTypeApiOps:
+ * - String formatting (FractionTimeFormatter)
+ * - Row encoding (LocalTimeEncoder)
+ *
+ * INTERNAL REPRESENTATION:
+ * - Values stored as Long nanoseconds since midnight
+ * - Range: 0 to 86,399,999,999,999
+ * - External type: java.time.LocalTime
+ * - Precision (0-6) affects display only, not storage
+ *
+ * @param t
+ * The TimeType with precision information
+ * @since 4.2.0
+ */
+case class TimeTypeOps(override val t: TimeType) extends TimeTypeApiOps(t) with TypeOps {
+
+ override def dataType: DataType = t
+
+ // ==================== Physical Type Representation ====================
+
+ override def getPhysicalType: PhysicalDataType = PhysicalLongType
+
+ override def getJavaClass: Class[_] = classOf[Long]
+
+ override def getMutableValue: MutableValue = new MutableLong
+
+ override def getRowWriter(ordinal: Int): (InternalRow, Any) => Unit =
+ (input, v) => input.setLong(ordinal, v.asInstanceOf[Long])
+
+ // ==================== Literal Creation ====================
+
+ override def getDefaultLiteral: Literal = Literal.create(0L, t)
+
+ override def getJavaLiteral(v: Any): String = s"${v}L"
+
+ // ==================== External Type Conversion ====================
+
+ override def toCatalystImpl(scalaValue: Any): Any = {
+ DateTimeUtils.localTimeToNanos(scalaValue.asInstanceOf[LocalTime])
+ }
+
+ override def toScala(catalystValue: Any): Any = {
+ if (catalystValue == null) null
+ else DateTimeUtils.nanosToLocalTime(catalystValue.asInstanceOf[Long])
+ }
+
+ override def toScalaImpl(row: InternalRow, column: Int): Any = {
+ DateTimeUtils.nanosToLocalTime(row.getLong(column))
Review Comment:
The check is not needed because it the method is called for non-null columns only. See:
https://github.com/apache/spark/blob/aff735caa3d9c1d26b3941ae961509fa4b95edc5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystTypeConverters.scala#L149-L151
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2852900425
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -71,6 +72,9 @@ object RowEncoder extends DataTypeErrorsBase {
private[sql] def encoderForDataType(dataType: DataType, lenient: Boolean): AgnosticEncoder[_] =
dataType match {
+ // Types Framework: delegate to EncodeTypeOps for supported types when enabled
+ case _ if SqlApiConf.get.typesFrameworkEnabled && EncodeTypeOps.supports(dataType) =>
Review Comment:
makes sense, will move it either to `supports` or `apply`, depending on decisions from other comments on whether we want to merge them or not
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2851994689
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TypeApiOps.scala:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.SparkException
+import org.apache.spark.sql.types.{DataType, TimeType}
+
+/**
+ * Base trait for client-side (spark-api) type operations.
+ *
+ * PURPOSE: TypeApiOps handles operations that require spark-api internals (e.g., AgnosticEncoder)
+ * that are not available in the catalyst package. This separation prevents circular dependencies
+ * between sql/api and sql/catalyst modules.
+ *
+ * USAGE - examples of use:
+ * - Row encoding/decoding (EncodeTypeOps)
+ * - String formatting (FormatTypeOps)
+ *
+ * RELATIONSHIP TO TypeOps:
+ * - TypeOps (catalyst): Server-side operations - physical types, literals, conversions
+ * - TypeApiOps (spark-api): Client-side operations - encoding, formatting
+ *
+ * For TimeType, TimeTypeOps extends TimeTypeApiOps to inherit both sets of operations.
+ *
+ * @see
+ * TimeTypeApiOps for a reference implementation
+ * @since 4.1.0
Review Comment:
should be 4.2.0
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871997406
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TypeApiOps.scala:
##########
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types.ops
+
+import org.apache.spark.sql.catalyst.encoders.AgnosticEncoder
+import org.apache.spark.sql.internal.SqlApiConf
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.unsafe.types.UTF8String
+
+/**
+ * Client-side (spark-api) type operations for the Types Framework.
+ *
+ * This trait consolidates all client-side operations that a data type must implement to be usable
+ * in the Spark SQL API layer. All methods are mandatory because a type cannot function correctly
+ * without string formatting (needed for CAST to STRING, EXPLAIN, SHOW) or encoding (needed for
+ * Dataset[T] operations).
+ *
+ * This single-interface design was chosen over separate FormatTypeOps/EncodeTypeOps traits to
+ * make it clear what a new type must implement - there is one mandatory interface, and it
+ * contains everything required. Optional capabilities (e.g., proto, Arrow, JDBC) are defined as
+ * separate traits that can be mixed in incrementally.
+ *
+ * RELATIONSHIP TO TypeOps:
+ * - TypeOps (catalyst): Server-side operations - physical types, literals, conversions
+ * - TypeApiOps (spark-api): Client-side operations - formatting, encoding
+ *
+ * The split exists because sql/api cannot depend on sql/catalyst. For TimeType, TimeTypeOps
+ * (catalyst) extends TimeTypeApiOps (sql-api) to inherit both sets of operations.
+ *
+ * @see
+ * TimeTypeApiOps for reference implementation
+ * @since 4.2.0
+ */
+trait TypeApiOps extends Serializable {
+
+ /** The DataType this Ops instance handles. */
+ def dataType: DataType
+
+ // ==================== String Formatting ====================
+
+ /**
+ * Formats an internal value as a display string.
+ *
+ * Used by CAST to STRING, EXPLAIN output, SHOW commands.
+ *
+ * @param v
+ * the internal value (e.g., Long nanoseconds for TimeType)
+ * @return
+ * formatted string (e.g., "10:30:45.123456")
+ */
+ def format(v: Any): String
Review Comment:
actually, looking at it now there's no reason to not add it immediately, it fits purposefully into this PR... adding it
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871361782
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -70,62 +71,63 @@ object RowEncoder extends DataTypeErrorsBase {
}
private[sql] def encoderForDataType(dataType: DataType, lenient: Boolean): AgnosticEncoder[_] =
Review Comment:
nit: we can organize the code in a way to reduce diff
```
private[sql] def encoderForDataType(dataType: DataType, lenient: Boolean): AgnosticEncoder[_] =
TypeApiOps(dataType).map(_.getEncoder).getOrElse(encoderForDataTypeLegacy(dataType, lenient))
private def encoderForDataTypeLegacy(dataType: DataType, lenient: Boolean): AgnosticEncoder[_] =
// the original implementation
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55440][SQL] Types Framework - Phase 1a - Core Type System Foundation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54223:
URL: https://github.com/apache/spark/pull/54223#discussion_r2871379651
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimeTypeOps.scala:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.types.ops
+
+import java.time.LocalTime
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.catalyst.expressions.{Literal, MutableLong, MutableValue}
+import org.apache.spark.sql.catalyst.types.{PhysicalDataType, PhysicalLongType}
+import org.apache.spark.sql.catalyst.util.DateTimeUtils
+import org.apache.spark.sql.types.{DataType, TimeType}
+import org.apache.spark.sql.types.ops.TimeTypeApiOps
+
+/**
+ * Server-side (catalyst) operations for TimeType.
+ *
+ * This class implements all TypeOps methods for the TIME data type, providing:
+ * - Physical type: PhysicalLongType (nanoseconds since midnight)
+ * - Literals: default is 0L (midnight)
+ * - External conversion: java.time.LocalTime <-> Long nanoseconds
+ *
+ * It also inherits client-side operations from TimeTypeApiOps:
+ * - String formatting (FractionTimeFormatter)
+ * - Row encoding (LocalTimeEncoder)
+ *
+ * INTERNAL REPRESENTATION:
+ * - Values stored as Long nanoseconds since midnight
+ * - Range: 0 to 86,399,999,999,999
+ * - External type: java.time.LocalTime
+ * - Precision (0-6) affects display only, not storage
+ *
+ * @param t
+ * The TimeType with precision information
+ * @since 4.2.0
+ */
+case class TimeTypeOps(override val t: TimeType) extends TimeTypeApiOps(t) with TypeOps {
+
+ override def dataType: DataType = t
Review Comment:
do we need this line? The client side `TimeTypeApiOps` already does it
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org