[SPARK-54683][SQL] Unify geo and time types blocking #53438
Conversation
| } | ||
|
|
||
| create.tableSchema.foreach(f => TypeUtils.failWithIntervalType(f.dataType)) | ||
| TypeUtils.failUnsupportedDataType(create.tableSchema, SQLConf.get) |
There was a problem hiding this comment.
This is a bit off topic, but just follow how we block interval type for v2 catalogs, which we don't have any builtin impl in Spark yet.
There was a problem hiding this comment.
Could you use a new JIRA ID instead of a follow-up, @cloud-fan ?
cloud-fan
changed the title
| createDeserializerForInstant(path) | ||
| case LocalDateTimeEncoder => | ||
| createDeserializerForLocalDateTime(path) | ||
| case LocalTimeEncoder if !SQLConf.get.isTimeTypeEnabled => |
There was a problem hiding this comment.
Is this missed before? Or a side-effect of unification (which becomes to miss in other layer)?
There was a problem hiding this comment.
It's missed before and I found it when trying to unify the geo type blocking code.
| case TimestampEncoder(false) => createSerializerForSqlTimestamp(input) | ||
| case InstantEncoder(false) => createSerializerForJavaInstant(input) | ||
| case LocalDateTimeEncoder => createSerializerForLocalDateTime(input) | ||
| case LocalTimeEncoder if !SQLConf.get.isTimeTypeEnabled => |
| case LocalDateTimeEncoder => | ||
| createDeserializerForLocalDateTime(path) | ||
| case LocalTimeEncoder if !SQLConf.get.isTimeTypeEnabled => | ||
| throw org.apache.spark.sql.errors.QueryCompilationErrors.unsupportedTimeTypeError() |
There was a problem hiding this comment.
Shall we import org.apache.spark.sql.errors.QueryCompilationErrors?
There was a problem hiding this comment.
geo type blocking does not import org.apache.spark.sql.AnalysisException either...
It's temporary and will be removed after we complete time type development. I think it's fine.
| case InstantEncoder(false) => createSerializerForJavaInstant(input) | ||
| case LocalDateTimeEncoder => createSerializerForLocalDateTime(input) | ||
| case LocalTimeEncoder if !SQLConf.get.isTimeTypeEnabled => | ||
| throw org.apache.spark.sql.errors.QueryCompilationErrors.unsupportedTimeTypeError() |
There was a problem hiding this comment.
ditto. import org.apache.spark.sql.errors.QueryCompilationErrors?
|
Could you fix the compilation error, @cloud-fan ? |
|
Given the current status, I believe we are able to merge this as 4.1.1 instead of blocking I'm going to proceed toward RC3 from the AS-IS |
|
yea it doesn't block 4.1 RC as time type is mostly blocked and users won't be able to use it in any meaningful workloads. |
|
Thank you, @cloud-fan . |
|
The test failure in |
|
merging to master/4.1 (RC3 has a outstanding -1 for memory leak) |
This PR aims to refactor the code that blocks time and geo types. code unification no existing tests no Closes #53438 from cloud-fan/block. Lead-authored-by: Wenchen Fan <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 4a18179) Signed-off-by: Wenchen Fan <[email protected]>
|
+1, LGTM. Thank you, @cloud-fan and all. |
3 participants
What changes were proposed in this pull request?
This PR aims to refactor the code that blocks time and geo types.
Why are the changes needed?
code unification
Does this PR introduce any user-facing change?
no
How was this patch tested?
existing tests
Was this patch authored or co-authored using generative AI tooling?
no