Repository navigation
[GLUTEN-13450][FLINK][VL] Support TRIM/LTRIM/RTRIM/LPAD/RPAD to the velox flinksql functions. - #13460
Conversation
|
@KevinyhZou @lgbo-ustc @philo-he @ggjh-159 Hi all, would you mind reviewing this PR? |
ggjh-159
left a comment
ggjh-159
left a comment
There was a problem hiding this comment.
Wire the development-phase dependency chain into this PR: .github/workflows/flink.yml still clones bigo-sg/velox4j at gluten-20260829, which predates the velox ref update this change depends on, so the stack CI builds has no flinksql trim/pad functions and the new e2e tests cannot pass there. During development, point the clone at the velox4j branch carrying the ref update (git clone -b <velox4j-branch> <fork>/velox4j.git); once the velox PR is merged, switch back to the upstream repo and commit id.
…anner Map the five Flink string functions to the velox flinksql implementations: plain name mappings for LTRIM/RTRIM/LPAD/RPAD; a dedicated TRIM converter dispatching on the Calcite trim flag (LEADING/TRAILING/BOTH) and reordering the operands to (str, seek). Adds TrimPadFunctionsTest with 9 end-to-end cases.
Companion change to the velox review feedback: Flink's BinaryStringDataUtil.isSpaceString checks only byteAt(0) for segment-backed (column- or expression-sourced) seeks, degrading a seek that starts with a space to space-only trimming, while plan constants keep the character-set walk. TRIM/LTRIM/RTRIM now route by the seek operand's constancy: literals map to trim/ltrim/rtrim, runtime seeks to trim_runtime/ltrim_runtime/rtrim_runtime. LTRIM/RTRIM move to a TrimSeekRexCallConverter (the one-argument form still maps directly). Tests cover both flavors, including the reviewer's RTRIM('abx ', ' x') case.
b3ddd70 to
6b6b6e6
Compare
|
please update velox4j version and Flink.md. @AnissaLiu |
…ql trim ref Point the Flink CI's velox4j checkout at cc8b56d, which carries the velox ref at ee61a637b — the merged flinksql string functions (bigo-sg/velox#89) that TrimPadFunctionsTest's runtime path requires. docs/Flink.md is updated to match.
Okay! The latest Velox4J merge commit ID has now been updated here :) |
What changes are proposed in this pull request?
Fixes #13450
Maps the Flink scalar functions TRIM/LTRIM/RTRIM/LPAD/RPAD to the velox
flinksql implementations:
1:
TRIM→TrimRexCallConverter: Calcite normalizes every TRIM call to three operands(flag, seek, str)(e.g.TRIM(s)becomes(BOTH, ' ', s)); the converter validates the flag literal, maps LEADING/TRAILING/BOTH toltrim/rtrim/trim, and reorders the operands to(str, seek). A non-literal flag is rejected inisSuitable.2:
LTRIM/RTRIM→TrimSeekRexCallConverter(one-argument form maps directly);LPAD/RPAD→ plain name mappings.3: Seek routing mirrors Flink's
BinaryStringDataUtil.isSpaceString(BinaryStringDataUtil.java:967-973): a plan-constant seek keeps its Java object and takes the character-set walk (trim/ltrim/rtrim), while a runtime-sourced seek (column or computed expression) is segment-backed and Flink checks only its first byte — a seek starting with a space degrades to space-only trimming (trim_runtime/ltrim_runtime/rtrim_runtime).Note: the velox side (flinksql registrations of the same five functions) is prepared in a separate velox PR (bigo-sg/velox#89); the runtime path of the new e2e tests requires that native support, so the two PRs are sequenced accordingly.
How was this patch tested?
TrimPadFunctionsTest(10 cases) ingluten-flink/ut: end-to-end on the velox backend — one-arg trim family with tab preservation and all-space rows, the three TRIM flags with both literal and column seek, both seek flavors for the two-argument forms (includingRTRIM('abx ', ' x')→abxfor a column seek versusabfor a literal seek), LPAD/RPAD boundaries (negative len → NULL, len 0, empty pad → NULL, truncation, multi-char pad cycling), NULL propagation and nestedLPAD(TRIM(c), 15, '*'). All 10 pass.RexNodeConverterTestregression: 2/2 pass.