sunshone commented on code in PR #13470:
URL: https://github.com/apache/gluten/pull/13470#discussion_r4513217959
##########
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:
@ggjh-159 Thanks a lot for catching this — both are whitelisted now, at the
branch tip.
IS [NOT] NULL and SEARCH are added to the carrier-predicate whitelist
alongside the comparisons: a nullability check and a point / range search on
the INTEGER milliseconds-of-day carrier keep TIME semantics, and both lower to
the existing converters — IS [NOT] NULL to isnull / isnotnull over the carrier,
and SEARCH (the Calcite form of IN) to in / between over the carrier values.
Getting SEARCH to work end-to-end also needed the range-bound conversion to
handle TIME: Calcite holds TIME bounds as TimeString, and they now convert to
the millisecond-of-day carrier the same way TIME literals do.
The allow/reject matrix pins both: a new planner test covers IS NULL / IS
NOT NULL over a TIME-typed InputRef (with the isnull / isnotnull function names
asserted) and a SEARCH over a TIME column with a two-point Sarg (lowered to in
over the carrier points), and the full planner and execution suites pass on
this branch (83 cases, 0 failures).
--
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]
