[SPARK-52770][CONNECT] Support TIME type in connect proto

[peter-toth]

What changes were proposed in this pull request?

This PR adds TIME type support to Spark Connect.

Why are the changes needed?

TIME type is new in 4.1 and Spark Connect should support it.

Does this PR introduce any user-facing change?

Yes, it adds basic TIME type support.

How was this patch tested?

TODO.

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

No.

[MaxGekk]

@dengziming Could you review this PR since you are working on similar one: #51462

[dengziming]

Thank you @peter-toth for this PR, I think we should add a test case for this change, and I don't think your test case makes sense, I duplicated it locally and it run smoothly even without this change, take a look at the image below.

[dengziming]

Should we make precision optional analogous to Decimal.

[dongjoon-hyun]

Thank you, @peter-toth .

[dongjoon-hyun]

BTW, the PR description looks wrong to me.

# How was this patch tested?

Manually:

scala> sql("SELECT TIME '12:13:14'").show()
+---------------+
|TIME '12:13:14'|
+---------------+
|       12:13:14|
+---------------+

As reported on the JIRA issue, show is handled by differently and works already. collect was the problem.

$ bin/spark-connect-shell --remote sc://localhost:15002
Welcome to
      ____              __
     / __/__  ___ _____/ /__
    _\ \/ _ \/ _ `/ __/  '_/
   /___/ .__/\_,_/_/ /_/\_\   version 4.1.0-preview1
      /_/

Type in expressions to have them evaluated.
Spark connect server version 4.1.0-preview1.
Spark session available as 'spark'.

scala> sql("SELECT TIME '12:13:14'").show()
+---------------+
|TIME '12:13:14'|
+---------------+
|       12:13:14|
+---------------+

scala> sql("SELECT TIME '12:13:14'").collect()
org.apache.spark.SparkException: org.apache.spark.sql.connect.common.InvalidPlanInput: [INTERNAL_ERROR] Does not support convert time(6) to connect proto types. SQLSTATE: XX000

[dongjoon-hyun]

Please add a test case like the above comment.

[dengziming]

The difference is that we need to return the result schema which is represented by DataType, when calling show the schema is converted to StringType so it will work well, whereas when calling collect we need TimeType, this can't be tested by the tests I provided before.

-- sql("SELECT TIME '12:13:14'").show()
root {
  show_string {
    input {
      sql {
        query: "SELECT TIME \'12:13:14\'"
      }
    }
    num_rows: 20
    truncate: 20
    vertical: true
  }
}

-- sql("SELECT TIME '12:13:14'").collect()
root {
  sql {
    query: "SELECT TIME \'12:13:14\'"
  }
}

[peter-toth]

@dongjoon-hyun, ahh indeed, I ran the wrong test. The proper type support needs a bit more changes.

The difference is that we need to return the result schema which is represented by DataType, when calling show the schema is converted to StringType so it will work well, whereas when calling collect we need TimeType, this can't be tested by the tests I provided before.

@dengziming, that's expected.

[dongjoon-hyun]

Thank you for updating the PR, @peter-toth .

[peter-toth]

Thank you for updating the PR, @peter-toth .

Sure. I will add some more tests and adjust the description as well, but I don't see yet what would be the best place for the sql("SELECT TIME '12:13:14'").collect() test.

[dongjoon-hyun]

Just for my understanding. Do you know where is 25, 26, 27, @peter-toth ?

[peter-toth]

Unfortunately these are not in order. A few lines up:

  Variant variant = 25;

and down:

  // Reserved for geometry and geography types
  reserved 26, 27;

[dongjoon-hyun]

+1, LGTM from my side. Thank you, @peter-toth .

cc @MaxGekk , @grundprinzip , @HyukjinKwon

[HyukjinKwon]

cc @MaxGekk @hvanhovell @grundprinzip FYI

LGTM

[dengziming]

Just a stupid question, when is these method used? why should we change arrow related method when adding connect dataType?