Copilot commented on code in PR #12229:
URL: https://github.com/apache/gluten/pull/12229#discussion_r3377322040
##########
cpp/velox/substrait/SubstraitParser.cc:
##########
@@ -78,6 +77,8 @@ TypePtr SubstraitParser::parseType(const ::substrait::Type&
substraitType, bool
return DATE();
case ::substrait::Type::KindCase::kTimestampTz:
return TIMESTAMP();
+ case ::substrait::Type::KindCase::kTimestamp:
+ return TIMESTAMP_UTC();
case ::substrait::Type::KindCase::kDecimal: {
Review Comment:
`TIMESTAMP_UTC()` is referenced but not defined anywhere in the C++
codebase, which will fail compilation. If Substrait `timestamp` is intended to
map to Velox's timestamp type, use `TIMESTAMP()` here (and handle any NTZ vs
LTZ semantics elsewhere).
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -828,5 +828,5 @@ object VeloxConfig extends ConfigRegistry {
"containing TimestampNTZ will fall back to Spark execution. Set to
false during " +
"development/testing of TimestampNTZ support to allow native
execution.")
.booleanConf
Review Comment:
This docstring claims "When true (default)" but the config is now created
with default `false`. Please update the docstring to match the actual default
(or revert the default) to avoid misleading users/operators.
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -111,7 +111,10 @@ object VeloxValidatorApi {
StringType | BinaryType | _: DecimalType | DateType | TimestampType |
YearMonthIntervalType.DEFAULT | NullType =>
true
- case dt if !enableTimestampNtzValidation && dt.catalogString ==
"timestamp_ntz" => true
+ case dt if !enableTimestampNtzValidation && dt.catalogString ==
"timestamp_ntz" =>
+ // Allow TimestampNTZ when validation is disabled (for
development/testing)
+ // Use reflection to avoid compile-time dependency on Spark 3.4+
TimestampNTZType
+ true
Review Comment:
The comment mentions using reflection to avoid a compile-time dependency on
Spark's TimestampNTZType, but this branch only checks `dt.catalogString` (a
String) and doesn't use reflection. Please fix/remove the comment to avoid
confusion when maintaining this validation logic.
##########
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))
+ } catch {
+ case _: ClassNotFoundException =>
+ throw new GlutenNotSupportException(s"Type $substraitType not
supported.")
+ }
Review Comment:
The reflection block can fail with exceptions other than
ClassNotFoundException (e.g., NoSuchFieldException, IllegalAccessException).
Catching only ClassNotFoundException can crash plan conversion instead of
producing a clean GlutenNotSupportException fallback.
##########
cpp/velox/substrait/SubstraitToVeloxExpr.cc:
##########
@@ -133,6 +132,8 @@ TypePtr getScalarType(const
::substrait::Expression::Literal& literal) {
return DATE();
case ::substrait::Expression_Literal::LiteralTypeCase::kTimestampTz:
return TIMESTAMP();
+ case ::substrait::Expression_Literal::LiteralTypeCase::kTimestamp:
+ return TIMESTAMP_UTC();
case ::substrait::Expression_Literal::LiteralTypeCase::kString:
Review Comment:
`TIMESTAMP_UTC()` is referenced but not defined anywhere in the C++
codebase, which will fail compilation. Replace it with `TIMESTAMP()` (Velox
timestamp) unless there is an actual custom UTC timestamp type available.
##########
gluten-substrait/src/main/java/org/apache/gluten/substrait/expression/TimestampNTZLiteralNode.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.substrait.expression;
+
+import org.apache.gluten.substrait.type.TimestampTypeNode;
+import org.apache.gluten.substrait.type.TypeNode;
+
+import io.substrait.proto.Expression.Literal.Builder;
+
+public class TimestampNTZLiteralNode extends LiteralNodeWithValue<Long> {
+ public TimestampNTZLiteralNode(Long value) {
+ super(value, new TimestampTypeNode(true));
+ }
Review Comment:
TimestampNTZLiteralNode declares its type as TimestampTypeNode (Substrait
timestamp_tz) but writes the value into the `timestamp` (no-tz) literal field.
This type/value mismatch can produce invalid Substrait literals for
TIMESTAMP_NTZ and break downstream parsing.
--
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]
