Copilot commented on code in PR #12229:
URL: https://github.com/apache/gluten/pull/12229#discussion_r3393754588
##########
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala:
##########
@@ -703,6 +703,10 @@ class ClickHouseTestSettings extends BackendTestSettings {
.excludeCH("SPARK-31896: Handle am-pm timestamp parsing when hour is
missing")
.excludeCH("UNIX_SECONDS")
.excludeCH("TIMESTAMP_SECONDS")
+ // TimestampNTZ evaluation is not supported.
+ .excludeCH("Seconds")
+ .excludeCH("Minute")
+ .excludeGlutenTest("Gluten - Hour")
Review Comment:
`excludeGlutenTest` automatically prepends the `Gluten - ` prefix to the
provided test name. Passing an already-prefixed string here results in trying
to exclude `Gluten - Gluten - Hour`, so the intended Gluten test (`Gluten -
Hour`) will still run.
##########
gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala:
##########
@@ -703,6 +703,10 @@ class ClickHouseTestSettings extends BackendTestSettings {
.excludeCH("SPARK-31896: Handle am-pm timestamp parsing when hour is
missing")
.excludeCH("UNIX_SECONDS")
.excludeCH("TIMESTAMP_SECONDS")
+ // TimestampNTZ evaluation is not supported.
+ .excludeCH("Seconds")
+ .excludeCH("Minute")
+ .excludeGlutenTest("Gluten - Hour")
Review Comment:
`excludeGlutenTest` already prefixes test names with `Gluten - `. Using
`excludeGlutenTest("Gluten - Hour")` won’t match any test and won’t exclude the
intended Gluten test case.
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala:
##########
@@ -716,6 +716,10 @@ class ClickHouseTestSettings extends BackendTestSettings {
.excludeCH("SPARK-31896: Handle am-pm timestamp parsing when hour is
missing")
.excludeCH("UNIX_SECONDS")
.excludeCH("TIMESTAMP_SECONDS")
+ // TimestampNTZ evaluation is not supported.
+ .excludeCH("Seconds")
+ .excludeCH("Minute")
+ .excludeGlutenTest("Gluten - Hour")
Review Comment:
`excludeGlutenTest` prepends `Gluten - ` automatically. With the current
code, the exclusion target becomes `Gluten - Gluten - Hour`, so `Gluten - Hour`
will not be excluded as intended.
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxSQLQueryTestSettings.scala:
##########
@@ -58,7 +58,6 @@ object VeloxSQLQueryTestSettings extends SQLQueryTestSettings
{
"double-quoted-identifiers.sql",
"except.sql",
"except-all.sql",
- "extract.sql",
"group-by.sql",
"group-by-all.sql",
Review Comment:
Spark 3.5 Velox SQL query test settings no longer include `extract.sql`, but
other Spark versions’ Velox settings still do. This looks like an accidental
omission and reduces coverage for date/time extraction behavior.
##########
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxSQLQueryTestSettings.scala:
##########
@@ -180,8 +179,6 @@ object VeloxSQLQueryTestSettings extends
SQLQueryTestSettings {
"postgreSQL/window_part2.sql",
"postgreSQL/with.sql",
"datetime-special.sql",
- "timestamp-ltz.sql",
- "timestamp-ntz.sql",
"timezone.sql",
"transform.sql",
"try-string-functions.sql",
Review Comment:
Spark 3.5 Velox SQL query test settings are missing `timestamp-ltz.sql` and
`timestamp-ntz.sql` while Spark 3.3/3.4/4.0/4.1 Velox settings include them. If
these tests are expected to pass for Velox, they should be re-enabled here for
consistency and coverage.
##########
gluten-ut/spark33/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala:
##########
@@ -758,6 +758,8 @@ class ClickHouseTestSettings extends BackendTestSettings {
.excludeGlutenTest("unix_timestamp")
.excludeGlutenTest("to_unix_timestamp")
.excludeGlutenTest("Hour")
+ // TimestampNTZ evaluation is not supported.
+ .excludeGlutenTest("Gluten - Hour")
Review Comment:
`excludeGlutenTest` prefixes with `Gluten - ` internally, so
`excludeGlutenTest("Gluten - Hour")` won’t match any test. This suite already
excludes the Gluten Hour test via `.excludeGlutenTest("Hour")`, so this line is
redundant and should be removed to avoid confusion.
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/ConverterUtils.scala:
##########
@@ -160,6 +160,19 @@ object ConverterUtils extends Logging {
(StringType, isNullable(substraitType.getString.getNullability))
case Type.KindCase.BINARY =>
(BinaryType, isNullable(substraitType.getBinary.getNullability))
+ case Type.KindCase.TIMESTAMP =>
+ try {
+ (
+ Class
+ .forName("org.apache.spark.sql.types.TimestampNTZType$")
+ .getField("MODULE$")
+ .get(null)
+ .asInstanceOf[DataType],
+ isNullable(substraitType.getTimestamp.getNullability))
Review Comment:
With TIMESTAMP now mapped to Spark's TimestampNTZType, the type node flow
can produce `TimestampNTZTypeNode` instances (via
`TypeBuilder.makeTimestampNTZ`). However, `ConverterUtils.parseFromTypeNode`
does not handle `TimestampNTZTypeNode`, which will throw
`GlutenNotSupportException` when building list/map literals (see
`ListLiteralNode`/`MapLiteralNode` calling `parseFromTypeNode`). Please add a
`TimestampNTZTypeNode` case there (likely using the same reflection-based
TimestampNTZType lookup used here).
##########
gluten-ut/spark34/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala:
##########
@@ -657,6 +657,8 @@ class ClickHouseTestSettings extends BackendTestSettings {
.excludeGlutenTest("unix_timestamp")
.excludeGlutenTest("to_unix_timestamp")
.excludeGlutenTest("Hour")
+ // TimestampNTZ evaluation is not supported.
+ .excludeGlutenTest("Gluten - Hour")
Review Comment:
`excludeGlutenTest` automatically prefixes test names with `Gluten - `.
Passing a prefixed name here means the exclusion won’t match (`Gluten - Gluten
- Hour`). Since this suite already excludes the Gluten Hour test via
`.excludeGlutenTest("Hour")` above, this extra line is redundant and can be
removed to avoid confusion.
--
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]
