wbittles commented on code in PR #28153:
URL: https://github.com/apache/flink/pull/28153#discussion_r3303767893
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/sql/SqlWindowTableFunction.java:
##########
@@ -144,12 +144,21 @@ public static RelDataType inferRowType(
RelDataTypeFactory typeFactory,
RelDataType inputRowType,
RelDataType timeAttributeType) {
+ // window_start and window_end follow the input time attribute type
(TIMESTAMP or
+ // TIMESTAMP_LTZ) but strip the time attribute metadata
(rowtime/proctime markers)
+ RelDataType windowBoundType =
+ typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+ timeAttributeType.getSqlTypeName(),
+ timeAttributeType.getPrecision()),
+ false);
+
return typeFactory
.builder()
.kind(inputRowType.getStructKind())
.addAll(inputRowType.getFieldList())
- .add("window_start", SqlTypeName.TIMESTAMP, 3)
- .add("window_end", SqlTypeName.TIMESTAMP, 3)
+ .add("window_start", windowBoundType)
Review Comment:
Hi Apologises for the delay in responding, To be honest when I implemented
the changed advised aboe I was a bit daunted by the number of changes required
to the test cases , so I thought it best to just double check before proceeding
.
Apologises for the delay , but on accepting the recommendation offer of
propagating the type root, I became aware of the
scale of the changes and I thought it best to check on the best way forward.
Option 1: Propagate root type + precision
Window bounds (window_start, window_end) inherit both precision and logical
type from the input (TIMESTAMP vs TIMESTAMP_LTZ)
Ensures full type consistency (e.g. proctime → TIMESTAMP_LTZ)
Impact: ~57 test failures across ~20+ classes, requires widespread plan
updates, potential breaking change
Option 2: Propagate precision only (current behavior preserved)
Window bounds always use TIMESTAMP, but with correct precision
Fixes the precision issue while keeping existing type behavior
Impact: ~10 test errors across 2 classes, minimal updates required
Trade-off
Option 1: Semantically correct and consistent, but disruptive and
potentially breaking
Option 2: Backward-compatible and low-risk, but introduces inconsistency for
TIMESTAMP_LTZ inputs
Question:
What is the preferred direction here?
Should we move to Option 1 for semantic correctness (possibly gated behind a
major version change), or
Proceed with Option 2 as a pragmatic fix that preserves existing behavior?
Happy to proceed either way and follow up with the necessary changes once
direction is agreed.
--
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]
