mattcuento commented on PR #27386: URL: https://github.com/apache/flink/pull/27386#issuecomment-3961412268
> My point is that at the same time there are changes in flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/stream/StreamExecWindowTableFunction.java and we do not add tests within this PR for stream cases... Yep understandable. My point here is that Flink enforces that watermark attributes have a precision of 3 or less, meaning we can't encounter the outlined bug (precision must be > 3 in binary row data format). The reason for making the change in the `CommonExecWindowTableFunction` is that inheritance is used in `translateToPlanInternal` for batch. Rather than diverge into different implementations for stream vs. batch, I opted to keep the common implementation, to prevent such a bug from occurring in the future for streaming if greater precision of watermark attributes gets supported. > Do we already have tests covering this or how can we be sure it behaves as expected (after changes are done)? Also if it is not covered, do we need changes in stream at all? In essence, every existing window restore test for event and proc time gives us coverage for such a scenario. Coverage would need to be added if greater precision was supported, but we're unable to do so now with the enforcement of precision being <= 3 today. I could just create a `translateToPlanInternal` implementation in the batch node if that's preferred, I just figured we would want to ensure we have knowledge of precision across both node types. What would you prefer? -- 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]
