featzhang commented on code in PR #28153:
URL: https://github.com/apache/flink/pull/28153#discussion_r3236009618
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/sql/SqlWindowTableFunction.java:
##########
@@ -144,12 +144,20 @@ public static RelDataType inferRowType(
RelDataTypeFactory typeFactory,
RelDataType inputRowType,
RelDataType timeAttributeType) {
+ // window_start and window_end are always plain TIMESTAMP(3) without
time attribute
+ // metadata, regardless of whether the time attribute is rowtime or
proctime
+ RelDataType plainTimestampType =
+ typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+ SqlTypeName.TIMESTAMP,
timeAttributeType.getPrecision()),
+ false);
Review Comment:
This block contradicts the stated intent of the PR. The comment says
`window_start` and `window_end` are *always plain TIMESTAMP(3) regardless of
whether the time attribute is rowtime or proctime`, and the code hard-codes
`SqlTypeName.TIMESTAMP` — but FLINK-36665 and the `BuiltInFunctionDefinitions`
change in this same PR (switching to `argument(0)`) require the output type to
follow the input time attribute, i.e. `TIMESTAMP_LTZ` when the input is
`TIMESTAMP_LTZ` (proctime falls into this case as well). Only the precision is
being propagated here, not the type root.
This is consistent with the test failures: `SINK_TVF_SCHEMA` and
`SINK_TVF_AGG_SCHEMA` were changed to `TIMESTAMP_LTZ(3)`, but every program
that consumes them (`WINDOW_TABLE_FUNCTION_TUMBLE_TVF`, `..._OFFSET`,
`..._AGG`, the HOP / CUMULATE variants) calls `String.format(TUMBLE_TVF,
"bid_time")` — and `bid_time` is defined in `SOURCE` as `TO_TIMESTAMP(ts)`
(plain `TIMESTAMP(3)` rowtime), so the planner-produced
`window_start`/`window_end` columns can't be `TIMESTAMP_LTZ` for those
programs. The Azure run on this commit is already FAILURE, which is in line
with this mismatch.
Suggest aligning this with the existing `window_time` line just below — i.e.
just propagate `timeAttributeType` directly:
```java
RelDataType windowBoundType =
typeFactory.createTypeWithNullability(timeAttributeType, false);
...
.add("window_start", windowBoundType)
.add("window_end", windowBoundType)
.add("window_time", windowBoundType)
```
Then the test schema bumps should be limited to the programs whose input
time attribute is actually `TIMESTAMP_LTZ` (currently only
`WINDOW_TABLE_FUNCTION_TUMBLE_TVF_UNION_ALL`, plus any new proctime program you
may want to add — proctime is `TIMESTAMP_LTZ(3)` and is not covered by the
current test set).
--
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]
