sunshone commented on PR #13470: URL: https://github.com/apache/gluten/pull/13470#issuecomment-6497490442
@lgbo-ustc Thank you very much for the thorough review — the dependency, correctness and coverage gaps are all addressed now. The dependency chain and landing order are documented in the updated PR description: velox#93 (native functions) -> velox4j#72 (ref bump, pinned to the velox PR branch) -> this PR, with the expected commit pairing (the velox branch tip 2c738d17b plus a follow-up formatting fix, and this PR's rework tip 50e6b1261 now; merged-commit ids as placeholders until the merges exist — no hash invented). The CI no longer builds a velox4j without the temporal natives: .github/workflows/flink.yml and gluten-flink/docs/Flink.md (commit 50e6b1261) now clone sunshone/velox4j at the flinksql-temporal-expressions branch and pin its tip, with a TODO note to revert to the canonical bigo-sg/velox4j gluten-20260829 pin (2d08448f...) once the velox-side functions land there — so the planner changes and the native runtime resolve the same functions in CI during review, and the revert is the only landing action left. The per-record semantics are fixed on the velox side (velox commit 2c738d17b; see the velox#93 replies for the RecordClock mechanism): same-row equality is asserted at three levels — velox gtest (U10: two independent current_timestamp call sites per-row equal; U11: current_timestamp == now; U6-U8 tightened to exact derivations), the checked-in streaming execution tests E1-E3 (same-row CURRENT_TIMESTAMP == NOW string equality at ms precision, LOCALTIME == exact ms-of-day of the same row's LOCALTIMESTAMP, UTC and Asia/Shanghai sessions, plus the filter-path variant), and the e2e dual-run same-row case (033), which matched byte-for-byte on both engines (7 x [id, true, true]). The test coverage is now checked in: RexNodeConverterTest covers the four mappings with function-name and result-type assertions (LOCALTIME -> IntegerType; NOW built through FlinkSqlOperatorTable.dynamicFunctions as the planner registers it at runtime) — 13/13 including the upstream literal-conversion cases that share the file after the rebase; a new LogicalTypeConverterTest covers TIME(p) -> INTEGER for p = 0/3/6/9 plus the TimestampType/TIMESTAMP_LTZ anchors (5/5); a new TemporalFunctionsTest (NexmarkTest-style streaming base, bounded values source, print sink) runs the family natively in UTC and Asia/Shanghai with LOCALTIME INTEGER carrier assertions (3/3). Every wall-clock assertion in the new tests is a bounded [t0-eps, t1+eps] window, never a fixed value, and the dual-run evidence is checked in as fixed-input SQL cases rather than manual classpath switching. No regression elsewhere in the ut module. -- 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]
