stevomitric commented on code in PR #56158:
URL: https://github.com/apache/spark/pull/56158#discussion_r3318883129
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -97,6 +99,10 @@ object RowEncoder extends DataTypeErrorsBase {
case TimestampType if SqlApiConf.get.datetimeJava8ApiEnabled =>
InstantEncoder(lenient)
case TimestampType => TimestampEncoder(lenient)
case TimestampNTZType => LocalDateTimeEncoder
+ // Nano timestamp types intentionally do not honor `lenient`: legacy
`java.sql.Timestamp` /
+ // `java.sql.Date` external types are out of scope for nanosecond
precision (SPARK-57033).
+ case t: TimestampNTZNanosType => LocalDateTimeNanosEncoder(t.precision)
+ case t: TimestampLTZNanosType => InstantNanosEncoder(t.precision)
Review Comment:
We can inject types here also? Or are you waiting for a refactor PR? Myabe
it would be a better idea to do it now so places like this one don't pile up in
the future.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystTypeConverters.scala:
##########
@@ -298,7 +300,7 @@ object CatalystTypeConverters {
}
new GenericInternalRow(ar)
case other => throw new SparkIllegalArgumentException(
- errorClass = "_LEGACY_ERROR_TEMP_3219",
+ errorClass = "INVALID_EXTERNAL_VALUE",
Review Comment:
Why rename the legacy error in this PR?
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -97,6 +99,10 @@ object RowEncoder extends DataTypeErrorsBase {
case TimestampType if SqlApiConf.get.datetimeJava8ApiEnabled =>
InstantEncoder(lenient)
case TimestampType => TimestampEncoder(lenient)
case TimestampNTZType => LocalDateTimeEncoder
+ // Nano timestamp types intentionally do not honor `lenient`: legacy
`java.sql.Timestamp` /
+ // `java.sql.Date` external types are out of scope for nanosecond
precision (SPARK-57033).
+ case t: TimestampNTZNanosType => LocalDateTimeNanosEncoder(t.precision)
Review Comment:
This looks like it bypasses the `spark.sql.timestampNanosTypes.enabled` gate.
`CatalystTypeConverters` still calls
`TypeUtils.failUnsupportedDataType(...)`, so schema-driven converter paths
reject nanos timestamp types when the feature is disabled. But `RowEncoder` now
maps `TimestampNTZNanosType` / `TimestampLTZNanosType` to nanos encoders
unconditionally, which means `Encoders.row(schema)` /
`ExpressionEncoder(schema)` can succeed with the flag off.
Can we add the same gate here (or centralize the unsupported-type check) and
add a negative test for the flag-off case?
##########
sql/core/src/test/java/test/org/apache/spark/sql/JavaDatasetSuite.java:
##########
@@ -789,6 +789,29 @@ public void testLocalDateTimeEncoder() {
Assertions.assertEquals(data, ds.collectAsList());
}
+ @Test
+ public void testTimestampNanosRowEncoder() {
+ spark.conf().set("spark.sql.timestampNanosTypes.enabled", "true");
Review Comment:
Do we need to explicitly set this? Its enabled in tests by default.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
