[SPARK-54609][SQL] Disable TIME type by default #53344

davidm-db · 2025-12-05T14:37:49Z

What changes were proposed in this pull request?

Introducing a new SQL config for TIME type: spark.sql.timeType.enabled.

The default value is false and it is enabled only in tests.

Why are the changes needed?

TIME data type support is not complete, so we need to guard it before it is completed, especially ahead of Spark 4.1 release.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Need to add tests for disabled config.

Was this patch authored or co-authored using generative AI tooling?

No.

dongjoon-hyun

Could you make CI happy, @davidm-db ?

dongjoon-hyun · 2025-12-05T17:32:33Z

cc @MaxGekk , @uros-db , @cloud-fan , @HyukjinKwon , @viirya , @peter-toth , @yaooqinn , @LuciferYang , @vinodkc

sql/api/src/main/scala/org/apache/spark/sql/types/TimeType.scala

cloud-fan · 2025-12-05T19:56:32Z

...re/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFileFormat.scala

  }

  override def supportDataType(dataType: DataType): Boolean = dataType match {
+    case _: TimeType => SQLConf.get. isTimeTypeEnabled


how do we block geo types for data sources?

Per offline discussion with @uros-db, blocking for Parquet should be sufficient for TIME.

Added guards for all file formats (that haven't previously been explicitly marked as not supported for TIME).

dongjoon-hyun · 2025-12-06T23:43:00Z

Could you answer the above comments and make this PR pass the CIs for further discussion, @davidm-db ?

yaooqinn · 2025-12-07T06:59:53Z

TimeType is marked as Unstable. Is this short-term prohibition actually required？

dongjoon-hyun · 2025-12-07T18:50:38Z

Yes, we need this to protect the users from the accidental use of unfinished work, @yaooqinn .

TimeType is marked as Unstable. Is this short-term prohibition actually required？

…p change)

sql/api/src/main/scala/org/apache/spark/sql/types/TimeType.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

cloud-fan · 2025-12-08T15:54:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

        val specialDate = convertSpecialDate(value, zoneId).map(Literal(_, DateType))
        specialDate.getOrElse(toLiteral(stringToDate, DateType))
-      case TIME => toLiteral(stringToTime, TimeType())
+      case TIME if conf.isTimeTypeEnabled => toLiteral(stringToTime, TimeType())


since we have the check here, we don't need to update SqlBaseParser.g4 to complicate things.

I replicated what Max did internally. I think the reason for this is:

the code you are commenting is handling literal types (statement example: SELECT TIME'10:00:00') and is done this way to fit into the existing error message format

{time_type_enabled}? guard in SqlBaseParser.g4 guards references to the TIME as a type and throws different class of errors, i.e. datatype unsupported (statement example: CREATE TABLE t(col TIME))

I don't know if we want to change this behavior or not, please share your thoughts.

...re/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFileFormat.scala

cloud-fan · 2025-12-08T15:56:11Z

can we add some test cases following the geo types blocking PRs?

uros-db

@davidm-db Should we add some tests, e.g.

e2e sql query tests with config turned off
blocking data sources like Parquet, CSV
data frames (classic and spark connect)
also, there are Scala suites for casting

dongjoon-hyun

BTW, given that @yaooqinn 's comment, while working on this PR, we need to build a consensus on this by sending out an email to dev@spark mailing list, @davidm-db , @uros-db , and @cloud-fan .

It would be enough to reply on RC2 email about the TIME type. Maybe, could you send out the decision clearly to the mailing list, please, @cloud-fan , because @MaxGekk is not in the loop yet?

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

dongjoon-hyun · 2025-12-08T19:17:31Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

          unsupportedType = ctx.literalType.getText,
          supportedTypes =
+            // TODO: Remove TIME from the list.
            Seq("DATE", "TIMESTAMP_NTZ", "TIMESTAMP_LTZ", "TIMESTAMP", "INTERVAL", "X", "TIME"),


Maybe, the following style?

- Seq("DATE", "TIMESTAMP_NTZ", "TIMESTAMP_LTZ", "TIMESTAMP", "INTERVAL", "X", "TIME"), + Seq("DATE", "TIMESTAMP_NTZ", "TIMESTAMP_LTZ", "TIMESTAMP", "INTERVAL", "X") ++ (if (conf.isTimeTypeEnabled) Seq("TIME") else None)

Yeah, will do this definitely. There are a lot of dependencies and TIME needs to be guarded in a lot of places, so for now I'm just trying to figure out what's needed to make the CI pass. Afterwards, I'll sort out the TODO comment. Hope to finish everything tomorrow!

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

davidm-db · 2025-12-09T21:42:32Z

@dongjoon-hyun @cloud-fan I think I've resolved all of the comments. I'll make sure tonight that after my latest changes all CIs are passing.
Tomorrow, I'll add some negative tests as Wenchen suggested (i.e. TIME really doesn't work when flag is disabled) and I think that should be it for this PR.

dongjoon-hyun · 2025-12-09T22:51:07Z

Thank you, @davidm-db .

cloud-fan · 2025-12-10T02:47:04Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/xml/XmlFileFormat.scala

  override def toString: String = "XML"

  override def supportDataType(dataType: DataType): Boolean = dataType match {
+    case _: TimeType => SQLConf.get.isTimeTypeEnabled


We have a narrow waist: https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSource.scala#L531

check where we call disallowWritingIntervals

is this covering everything? isn't this only for a write path? how do we handle blocking on the read path then?

I assume that the idea is to have a check in single place instead of doing it for each File Format, which makes complete sense. I'm just wondering if with what you suggested we are covering the same scope as with checks in File Formats (current state of the code)?

we can also block read path in DataSource.resolveRelation

oh DataSourceUtils.verifySchema is a better narrow waist for both read and write paths.

so I just have one additional question - if we go down this way (which makes sense on a high level), I think we might be missing to completely block the type. for example, if you take a look at ParquetFileFormat#supportDataType, it recursively calls supportDataType function in case the root type is Array/Map/Struct. from the top of my mind, I think the same holds for Xml, maybe for some others.

am I missing something or what I just said makes sense?

DataSourceUtils.verifySchema gets the full schema and we can do whatever we want, e.g.

if (schema.existsRecursively(_.isInstanceOf[TimeType])) fail ...

dongjoon-hyun

Is this still missing, @davidm-db and @cloud-fan ?

Need to add tests for disabled config.

dongjoon-hyun · 2025-12-10T23:48:47Z

I verified manually that it's blocked properly.

scala> spark.sql("CREATE TABLE t(c TIME)")
org.apache.spark.sql.catalyst.parser.ParseException:
[UNSUPPORTED_DATATYPE] Unsupported data type "TIME". SQLSTATE: 0A000
== SQL (line 1, position 18) ==
CREATE TABLE t(c TIME)
                 ^^^^

Given that, shall we proceed those test case as a follow-up, @davidm-db and @cloud-fan ?

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

…onf.scala

cloud-fan · 2025-12-11T01:09:59Z

I removed all client side checks as it's not very meaningful. People can use the TimeType class directly and there is no point to only block SQL. People can use their own Spark Connect client which is out of our control. I think the server side checks should be sufficient: we block time related functions, and we block query result with time type (either collect the rows or write to data sources). This is also how we block geo types.

dongjoon-hyun · 2025-12-11T02:06:22Z

So, is this the final status from your side, @cloud-fan ? Then, could you give your approval?

dongjoon-hyun · 2025-12-11T02:09:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystTypeConverters.scala

        new GeometryConverter(g)
      case DateType if SQLConf.get.datetimeJava8ApiEnabled => LocalDateConverter
      case DateType => DateConverter
+      case _: TimeType if !SQLConf.get.isTimeTypeEnabled =>


Just for the record, we don't have this for GeographyType|GeometryType.

We have it here, a few lines above

case _ @ (_: GeographyType | _: GeometryType) if !SQLConf.get.geospatialEnabled => throw new org.apache.spark.sql.AnalysisException( errorClass = "UNSUPPORTED_FEATURE.GEOSPATIAL_DISABLED", messageParameters = scala.collection.immutable.Map.empty)

dongjoon-hyun · 2025-12-11T02:10:27Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala

  def verifySchema(format: FileFormat, schema: StructType, readOnly: Boolean = false): Unit = {
+    if (!SQLConf.get.isTimeTypeEnabled && schema.existsRecursively(_.isInstanceOf[TimeType])) {
+      throw QueryCompilationErrors.unsupportedTimeTypeError()
+    }


Ditto. We don't have this for Geo*Type.

no data source supports geo types yet, so it's not needed for now. But to be future-proof we should check geo here as well.

dongjoon-hyun · 2025-12-11T02:11:54Z

sql/gen-sql-functions-docs.py


    """
+    print("Enabling TIME data type")
+    jspark.sql("SET spark.sql.timeType.enabled = true")


Do we have this for Geo*Type?

I'm also curious about why geo didn't fail here...

dongjoon-hyun

Got it.

+1, LGTM. Thank you so much, @cloud-fan .

dongjoon-hyun · 2025-12-11T02:55:10Z

Could you cancel all previously launched the GitHub Action CI in order to make run the last commit? It seems that the last commit didn't get the resource yet.

dongjoon-hyun · 2025-12-11T03:28:59Z

I manually verified the compilation and Scalalinter and both on and off behavior manually.

scala> sql("create table t(a TIME)").show()
org.apache.spark.sql.AnalysisException: [UNSUPPORTED_TIME_TYPE] The data type TIME is not supported. SQLSTATE: 0A000

dongjoon-hyun · 2025-12-11T03:30:33Z

Merged to master.

~~Could you make a backporting PR, @davidm-db and @cloud-fan ? There exist conflicts in branch-4.1.~~

Never mind. I resolved the conflicts and am testing locally on branch-4.1 now.

Introducing a new SQL config for TIME type: `spark.sql.timeType.enabled`. The default value is `false` and it is enabled only in tests. TIME data type support is not complete, so we need to guard it before it is completed, especially ahead of Spark 4.1 release. No. Need to add tests for disabled config. No. Closes #53344 from davidm-db/davidm-db/time-config. Lead-authored-by: David Milicevic <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 18a9435) Signed-off-by: Dongjoon Hyun <[email protected]>

dongjoon-hyun · 2025-12-11T03:44:11Z

Merged to branch-4.1, too.

Disable TIME type by default

4a43700

davidm-db force-pushed the davidm-db/time-config branch from 7818f04 to 4a43700 Compare December 5, 2025 14:38

github-actions bot added SQL CONNECT labels Dec 5, 2025

fix build break

5857c5d

davidm-db force-pushed the davidm-db/time-config branch from aa1ac43 to 5857c5d Compare December 5, 2025 15:39

dongjoon-hyun reviewed Dec 5, 2025

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-54609] Disable TIME type by default~~ [SPARK-54609][SQL] Disable TIME type by default Dec 5, 2025

cloud-fan reviewed Dec 5, 2025

View reviewed changes

sql/api/src/main/scala/org/apache/spark/sql/types/TimeType.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 5, 2025

View reviewed changes

Remove config check in constructor

3b38c82

davidm-db force-pushed the davidm-db/time-config branch from f58fbfa to 02c68f0 Compare December 8, 2025 10:16

Remove unused import (CI complaining)

c861aa6

davidm-db force-pushed the davidm-db/time-config branch from 02c68f0 to c861aa6 Compare December 8, 2025 10:33

Return TIME to the list of supported types for the error message (tem…

b868e86

…p change)

cloud-fan reviewed Dec 8, 2025

View reviewed changes

sql/api/src/main/scala/org/apache/spark/sql/types/TimeType.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 8, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 8, 2025

View reviewed changes

...re/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFileFormat.scala Outdated Show resolved Hide resolved

uros-db reviewed Dec 8, 2025

View reviewed changes

dongjoon-hyun previously requested changes Dec 8, 2025

View reviewed changes

dongjoon-hyun reviewed Dec 8, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Dec 8, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Outdated Show resolved Hide resolved

davidm-db force-pushed the davidm-db/time-config branch from f2552ce to c9cf94c Compare December 8, 2025 23:26

Add config guard for all file formats

c5ae1e3

cloud-fan reviewed Dec 10, 2025

View reviewed changes

davidm-db added 2 commits December 10, 2025 21:47

Simplify file formats check if TIME is enabled

b5aafa3

Remove leftover imports

41c5936

dongjoon-hyun reviewed Dec 10, 2025

View reviewed changes

cloud-fan added 4 commits December 11, 2025 09:01

Update SqlBaseParser.g4

ad6dd16

Update parsers.scala

64f5281

Update SqlApiConf.scala

c6a07f8

Update AstBuilder.scala

7d1b27b

cloud-fan reviewed Dec 11, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

cloud-fan added 2 commits December 11, 2025 09:06

Update DataSourceUtils.scala

85fa1b6

Update sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLC…

4a3b31d

…onf.scala

dongjoon-hyun reviewed Dec 11, 2025

View reviewed changes

cloud-fan approved these changes Dec 11, 2025

View reviewed changes

dongjoon-hyun approved these changes Dec 11, 2025

View reviewed changes

dongjoon-hyun closed this in 18a9435 Dec 11, 2025

cloud-fan mentioned this pull request Dec 11, 2025

[SPARK-54683][SQL] Unify geo and time types blocking #53438

Closed

[SPARK-54609][SQL] Disable TIME type by default #53344

[SPARK-54609][SQL] Disable TIME type by default #53344

Uh oh!

Conversation

davidm-db commented Dec 5, 2025 • edited by dongjoon-hyun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Dec 5, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidm-db Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Dec 6, 2025

Uh oh!

yaooqinn commented Dec 7, 2025

Uh oh!

dongjoon-hyun commented Dec 7, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidm-db Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cloud-fan commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uros-db left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidm-db commented Dec 9, 2025

Uh oh!

dongjoon-hyun commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidm-db Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

davidm-db commented Dec 5, 2025 •

edited by dongjoon-hyun

Loading

davidm-db Dec 8, 2025 •

edited

Loading

davidm-db Dec 9, 2025 •

edited

Loading

cloud-fan commented Dec 8, 2025 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

davidm-db Dec 10, 2025 •

edited

Loading

dongjoon-hyun commented Dec 10, 2025 •

edited

Loading

cloud-fan commented Dec 11, 2025 •

edited

Loading

dongjoon-hyun commented Dec 11, 2025 •

edited

Loading