ggjh-159 commented on code in PR #13470:
URL: https://github.com/apache/gluten/pull/13470#discussion_r4512233707
##########
gluten-flink/planner/src/main/java/org/apache/gluten/rexnode/RexNodeConverter.java:
##########
@@ -61,12 +61,105 @@
/** Convertor to convert RexNode to velox TypedExpr */
public class RexNodeConverter {
+ // Flink TIME-typed values ride on the INTEGER milliseconds-of-day carrier.
+ // The designed carrier surfaces — the LOCALTIME / CURRENT_TIME direct
+ // projections, comparisons on the carrier, CAST between TIME-typed values
+ // and TIME literals (INTEGER constants) — keep consistent carrier
+ // semantics. Any other call whose own type or a direct operand is TIME
+ // would either fail against the velox signatures or silently evaluate the
+ // integer carrier as a plain number, so it is rejected here instead.
+ private static final Set<String> CARRIER_PROJECTION_OPERATORS =
+ Set.of("LOCALTIME", "CURRENT_TIME");
+
+ private static final Set<String> COMPARISON_OPERATORS = Set.of("=", "<>",
"<", "<=", ">", ">=");
+
+ private static final String TIME_UNSUPPORTED_SUFFIX =
+ "gluten-flink supports TIME-typed values only on the INTEGER
milliseconds-of-day"
+ + " carrier: the LOCALTIME / CURRENT_TIME direct projections,
comparisons"
+ + " on the carrier, CAST between TIME-typed values and TIME literals
keep"
+ + " carrier semantics; other TIME expressions (arithmetic, EXTRACT,
CAST"
+ + " to or away from TIME across other types, CASE, ...) lose TIME"
+ + " semantics on the INTEGER carrier and are rejected.";
+
+ /**
+ * Rejects a RexCall that would silently integer-ize TIME semantics. Rules:
the LOCALTIME /
+ * CURRENT_TIME carrier projections, comparisons and CAST between TIME-typed
values pass (they
+ * keep carrier semantics); any other call whose result type or any direct
operand type is TIME
+ * fails with an explicit planner-time error.
+ */
+ private static void maybeRejectTimeTypedCall(RexCall rexCall) {
+ String operatorName = rexCall.getOperator().getName();
+ if (CARRIER_PROJECTION_OPERATORS.contains(operatorName)) {
+ return;
+ }
+ boolean resultIsTime = rexCall.getType().getSqlTypeName() ==
SqlTypeName.TIME;
+ boolean hasTimeOperand = hasTimeOperand(rexCall);
+ if ("CAST".equals(operatorName)) {
+ // CAST between TIME-typed values keeps the carrier semantics (e.g.
+ // CAST(CURRENT_TIME AS TIME)); CAST to TIME from another type would
+ // lower to a velox cast(<type> AS INTEGER) that neither computes the
+ // milliseconds-of-day nor parses a time-of-day string, and CAST away
+ // from TIME (e.g. to VARCHAR) would expose the raw integer as the
+ // rendered value.
+ if (resultIsTime && !hasTimeOperand) {
+ throw new RuntimeException(
+ "Operator '"
+ + operatorName
+ + "' casts a non-TIME operand to TIME, which is not supported:
"
+ + TIME_UNSUPPORTED_SUFFIX
+ + " Offending node: "
Review Comment:
The guard whitelist passes comparisons by name but rejects IS [NOT] NULL and
SEARCH over a TIME-typed operand, both of which are carrier-isomorphic and
already have converters (isnull/isnotnull, SearchRexCallConverter in
RexCallConverterFactory). A nullable TIME column's `t IS NOT NULL` does not
fold away, worked through those converters before this change, and now fails
planning at the guard. Please add IS [NOT] NULL and SEARCH to the whitelist and
pin them in the RexNodeConverterTest allow/reject matrix.
--
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]
