KevinyhZou commented on code in PR #13375:
URL: https://github.com/apache/gluten/pull/13375#discussion_r4409286378
##########
gluten-flink/planner/src/main/java/org/apache/gluten/rexnode/TypeUtils.java:
##########
@@ -53,6 +53,20 @@ public static boolean isStringType(Type type) {
return type instanceof VarCharType;
}
+ // Matches the scalar signatures registered under the velox flinksql
+ // namespace: booleans, numerics, strings, dates, timestamps and decimals.
+ // BINARY, ARRAY, MAP and ROW have no flinksql registration and must be
+ // rejected at the converter level so the planner falls back instead of
Review Comment:
Same nit as on `ScalarOnlyRexCallConverter`: rejecting here does not make
the planner fall back; it causes plan-time failure via `getConverter`. Suggest
something like:
```
// rejected at the converter level so conversion fails at planning time
// instead of failing at runtime.
```
##########
gluten-flink/planner/src/main/java/org/apache/gluten/rexnode/RexNodeConverter.java:
##########
@@ -97,7 +97,41 @@ public static Type toType(RelDataType relDataType) {
return
LogicalTypeConverter.toVLType(FlinkTypeFactory.toLogicalType(relDataType));
}
+ private static Variant toNullVariant(SqlTypeName sqlTypeName) {
+ // A typed null needs no per-type carrier: the velox-side
+ // ConstantTypedExpr renders a null variant as a null constant of the
+ // EXPRESSION type (the variant kind is irrelevant on the null path), and
+ // the velox variant serde treats an absent "value" member as the typed
+ // null (matching velox4j's Include.NON_NULL serialization). VarCharValue
+ // is the natural null-accepting carrier.
+ switch (sqlTypeName) {
+ case BOOLEAN:
+ case TINYINT:
+ case SMALLINT:
+ case INTEGER:
+ case BIGINT:
+ case DOUBLE:
+ case FLOAT:
+ case VARCHAR:
+ case CHAR:
+ case BINARY:
+ case DECIMAL:
+ case DATE:
+ case INTERVAL_SECOND:
+ case SYMBOL:
+ case TIMESTAMP:
+ return new VarCharValue(null);
+ default:
+ throw new RuntimeException("Unsupported null rex node type: " +
sqlTypeName);
+ }
+ }
+
public static Variant toVariant(RexLiteral literal) {
+ if (literal.getValue() == null || literal.isNull()) {
+ // A typed null literal: velox4j's NON_NULL serialization omits the
+ // "value" member and the velox-side serde reads that as the typed null.
Review Comment:
Minor: velox4j now uses `@JsonInclude(JsonInclude.Include.ALWAYS)` for
`Variant`, so null carriers serialize as `"value": null` rather than omitting
the member. Velox accepts both; the comment is slightly outdated. Optional to
say that absent or explicit-null `value` both work.
##########
gluten-flink/planner/src/main/java/org/apache/gluten/rexnode/functions/ScalarOnlyRexCallConverter.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.rexnode.functions;
+
+import org.apache.gluten.rexnode.RexConversionContext;
+import org.apache.gluten.rexnode.RexNodeConverter;
+import org.apache.gluten.rexnode.TypeUtils;
+import org.apache.gluten.rexnode.ValidationResult;
+
+import io.github.zhztheplayer.velox4j.expression.CallTypedExpr;
+import io.github.zhztheplayer.velox4j.expression.TypedExpr;
+import io.github.zhztheplayer.velox4j.type.Type;
+
+import org.apache.calcite.rex.RexCall;
+
+import java.util.List;
+
+/**
+ * Converter for functions whose velox flinksql registrations cover scalar
types only. Complex
+ * operands (BINARY, ARRAY, MAP, ROW) are rejected here so the planner falls
back to the native
+ * operator instead of failing at runtime with an unresolved velox signature.
Review Comment:
Nit: this does not fall back to the native operator. When `isSuitable`
fails, `RexCallConverterFactory.getConverter` ends up with an empty converter
list and throws (`No suitable converter found for: IFNULL ...`), and
`StreamExecCalc.translateToPlanInternal` has no try/catch — so the job fails at
plan translation.
Suggest rewording, e.g.:
```
* operands (BINARY, ARRAY, MAP, ROW) are rejected here so conversion fails
* loudly at planning time instead of failing at runtime with an unresolved
* velox signature.
```
--
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]
