weiqingy commented on code in PR #2448:
URL: https://github.com/apache/auron/pull/2448#discussion_r3718190648
##########
auron-flink-extension/auron-flink-planner/src/main/java/org/apache/auron/flink/table/planner/converter/RexLiteralConverter.java:
##########
@@ -112,6 +112,37 @@ private static boolean isSupportedType(SqlTypeName
typeName) {
return SUPPORTED_TYPES.contains(typeName);
}
+ /**
+ * Builds a literal expression node from a plain {@link String} that is
not backed by a
+ * {@link RexNode}. Used for plan-time constants (such as a session
timezone read from
+ * configuration) that must travel to the native side as a string
argument. The value is
+ * serialized as a single-element {@code Utf8} Arrow record batch in IPC
stream format, matching
+ * the encoding {@link #convert} produces for CHAR/VARCHAR {@link
RexLiteral}s.
+ *
+ * @param value the constant string to encode
+ * @return a {@link PhysicalExprNode} carrying the value as a native
literal
+ */
+ public static PhysicalExprNode stringLiteral(String value) {
+ RowType rowType = RowType.of(new VarCharType(VarCharType.MAX_LENGTH));
+ try (BufferAllocator allocator =
+
FlinkArrowUtils.getRootAllocator().newChildAllocator("literal", 0,
Long.MAX_VALUE);
+ VectorSchemaRoot root =
VectorSchemaRoot.create(FlinkArrowUtils.toArrowSchema(rowType), allocator)) {
+
+ GenericRowData rowData = new GenericRowData(1);
+ rowData.setField(0, StringData.fromString(value));
+
Review Comment:
Added a `requireNonNull` in `cccb1acb`, though for a different reason than
the one given here.
Null cannot reach this method today. It has two call sites, both in
`RexCallConverter.buildUnixTimestamp`: one passes `chronoFormat`, which comes
out of `translate(...).orElseThrow(...)` and so is non-null by construction,
and the other passes `zone.getId()`, which `ZoneId` never returns null from.
On the diagnosis being opaque: conversion failures are already caught.
`FlinkNodeConverterFactory.convertRexNode` wraps the `convert` call in `catch
(Exception e)` and logs `RexNode conversion failed for {}` with the full stack
trace, then returns empty so the Calc falls back to Flink. So an NPE here would
surface as a logged stack trace plus a query that still returns correct results
on Flink's engine, not an opaque failure.
What I did take from this: the method is public API introduced by this PR,
and a null value would mean the caller failed to resolve a plan-time constant.
Encoding that as a NULL literal would ship a silently wrong argument to the
native side, so rejecting it at the boundary is the right contract to state.
Guard plus javadoc plus `testStringLiteralRejectsNull`.
--
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]
