philo-he commented on code in PR #13412:
URL: https://github.com/apache/gluten/pull/13412#discussion_r4535020407
##########
gluten-flink/planner/src/main/java/org/apache/gluten/rexnode/functions/RexCallConverterFactory.java:
##########
@@ -119,6 +120,14 @@ public class RexCallConverterFactory {
// substring returns an empty string. Start semantics (0 counts as 1,
// negative counts from the end) already match Flink.
Map.entry("SUBSTRING", Arrays.asList(() -> new
SubstringRexCallConverter())),
+ // Offload targets implemented in velox's flinksql namespace. ABS/
+ // FLOOR/CEIL/ROUND arrive upper-cased from the operator table;
+ // unary minus arrives as a one-operand "-" (see above).
+ Map.entry("ABS", Arrays.asList(() -> new
DefaultRexCallConverter("abs"))),
+ Map.entry("FLOOR", Arrays.asList(() -> new
NumericOnlyRexCallConverter("floor"))),
+ Map.entry("CEIL", Arrays.asList(() -> new
NumericOnlyRexCallConverter("ceil"))),
+ Map.entry("CEILING", Arrays.asList(() -> new
DefaultRexCallConverter("ceil"))),
Review Comment:
Nit: the `"CEILING"` entry is never used. Calcite parses `CEIL` and
`CEILING` into the same `SqlStdOperatorTable.CEIL` operator, whose name is
always `"CEIL"`, so `CEILING(x)` already goes through the `CEIL` entry (that's
what `testCeilingAlias` exercises). Could we drop the `"CEILING"` entry?
Keeping it with a different converter is misleading.
##########
gluten-flink/planner/src/main/java/org/apache/gluten/rexnode/functions/BaseRexCallConverters.java:
##########
@@ -89,6 +91,80 @@ public TypedExpr toTypedExpr(RexCall callNode,
RexConversionContext context) {
}
}
+/**
+ * Converter for the one-operand "-" RexCall (Flink's minusPrefix / SQL -x).
The name maps to the
+ * flinksql negate function; the result type equals the operand type, so no
cast is needed.
+ */
+class UnaryMinusRexCallConverter extends BaseRexCallConverter {
+ public UnaryMinusRexCallConverter(String functionName) {
+ super(functionName);
+ }
+
+ @Override
+ public ValidationResult isSuitable(RexCall callNode, RexConversionContext
context) {
+ if (callNode.getOperands().size() != 1) {
+ return ValidationResult.failure(
+ "Unary minus requires exactly one operand, but found: "
+ + getFunctionProtoTypeName(callNode));
+ }
+ // Numeric operand only: the interval form of unary minus would
+ // otherwise be accepted by the arity guard and fail later at
+ // toType (no interval type mapping); reject it at planning time.
+ // Decimals are accepted: velox's flinksql negate covers them.
+ try {
+ if (!org.apache.gluten.rexnode.TypeUtils.isNumericOrDecimalType(
Review Comment:
Nit: is there a reason to use the fully qualified
`org.apache.gluten.rexnode.TypeUtils` here instead of an import? An import
would be cleaner, especially if it may be used more than once in this file.
##########
gluten-flink/planner/src/main/java/org/apache/gluten/rexnode/functions/NumericOnlyRexCallConverter.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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 org.apache.calcite.rex.RexCall;
+
+import java.util.List;
+
+/**
+ * Converter for functions whose velox registrations cover numeric operands
only. Non-numeric
+ * operands are rejected here so conversion fails loudly at planning time
(e.g. the datetime form
+ * FLOOR(ts TO DAY) carries a SYMBOL literal that maps to VARCHAR and would
otherwise produce an
+ * unresolvable velox signature at runtime).
+ */
+class NumericOnlyRexCallConverter extends BaseRexCallConverter {
+
+ public NumericOnlyRexCallConverter(String functionName) {
+ super(functionName);
+ }
+
+ @Override
+ public ValidationResult isSuitable(RexCall callNode, RexConversionContext
context) {
+ try {
+ boolean allNumeric =
+ callNode.getOperands().stream()
+ .allMatch(
+ arg ->
TypeUtils.isNumericOrDecimalType(RexNodeConverter.toType(arg.getType())));
+ if (!allNumeric) {
+ return ValidationResult.failure(
+ String.format(
+ "Numeric-only operation requires all operands to be numeric or
decimal types, but found: %s",
+ getFunctionProtoTypeName(callNode)));
+ }
+ } catch (RuntimeException e) {
+ // Operand types without a velox mapping are rejected at planning
+ // time rather than blowing up in toType.
+ return ValidationResult.failure(
+ String.format(
+ "Numeric-only operation requires all operands to be numeric or
decimal types, but found: %s",
+ getFunctionProtoTypeName(callNode)));
Review Comment:
Nit: this error message duplicates the line 52. Can we use a single helper
for them?
--
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]
