Add mask functions to allow gandiva evaluation. - #143
Conversation
There was a problem hiding this comment.
🟡 Verdict: Minor issues found — 0 must fix, 1 should fix, 2 nits
This PR looks very good overall — the Hive-semantics alignment is well-evidenced, the new mask_internal is carefully bounds-checked, and the test coverage is unusually thorough. There are a couple of items I'd want to check (inline comment below); it is OK to resolve them as-is without code changes if you consider them out of scope or intentional — a short reply is enough.
🟡 Should fix:
- [Correctness]
mask_imploutput-size computation can overflow int32 for large text × long replacement — same int64 +INT32_MAXguard as ingdv_fn_mask_internalwould close it (cpp/src/gandiva/gdv_function_stubs.cc:576-580, see inline)
🔵 Nits (no code change requested):
-
The documented codepoint-vs-UTF-16 divergence is understated: it is visible not only at
char_countwindow boundaries. Verified against Dremio'sMaskTransformer.java(which iterates UTF-16 code units and classifies surrogate halves as "other"), FULL mode also diverges:mask_internal('😀','FULL',-1,'X','x','n','*')→ Java"**"vs native"*"(each surrogate unit takes the otherChar in Java)mask_internal('𐐀','FULL',-1,'X','x','n','-1')(U+10400 is Lu) → Java leaves it unchanged (surrogates = "other" = unmasked), native masks to"X"
In every divergent case the native path masks at least as much as Java, so there is no exposure risk — suggestion is only to widen the "Known divergence" note (stub comment + PR description say FULL mode is unaffected) and optionally pin the two cases above with one-line tests so the behavior is chosen rather than accidental.
-
Enhancement idea for a follow-up: since
modeand the replacement args are always literals (the Java side already requires@Param(constant = true)), aMaskInternalHoldermirroringToDateHolder(+kNeedsFunctionHolder) would hoist the per-row mode/replacement parsing to plan-build time — removes the per-row parsing overhead on short strings and turns an invalid mode literal into a plan-time error instead of a row-level runtime error.
Observation on pre-existing code (informational only, outside this PR's changes): the utf8proc_iterate calls in the older mask loops (mask_impl's utf8proc loop and gdv_mask_first_n/gdv_mask_last_n) pass the full data_len rather than the remaining length — the over-read your own comment in mask_internal_count_chars describes — and mask_impl's loop has no char_len < 0 check. The new gdv_fn_mask_internal handles both correctly. Could be a small follow-up ticket; no action requested here.
Change walkthrough (5 files)
| File | Change |
|---|---|
cpp/src/gandiva/function_registry_string.cc |
Registers mask_internal (7 args) and 5-arg mask with otherChar |
cpp/src/gandiva/gdv_function_stubs.cc |
Lu/Ll/Nd-only masking per Hive; shared mask_impl; new gdv_fn_mask_internal; LLVM mappings (verified to match the C signatures) |
cpp/src/gandiva/gdv_function_stubs.h |
Declarations for the two new exports |
cpp/src/gandiva/gdv_function_stubs_test.cc |
Six new test suites; empty-argument expectations updated |
cpp/src/gandiva/tests/projector_test.cc |
Three new end-to-end tests; four expectation updates for the intentional narrowing |
Generated with AI-assisted analysis
| std::max(upper_length, std::max(lower_length, num_length))) * | ||
| data_len; |
There was a problem hiding this comment.
🟡 SHOULD FIX · Correctness · High confidence
Int32 overflow in the output-size bound
max_repl * data_len is computed in 32-bit signed math and wraps negative for a large text value combined with a long replacement argument (both are user-suppliable SQL expressions). SimpleArena::Allocate then moves its cursor by a huge negative offset and returns a garbage pointer that the masking loops write through — heap corruption instead of a clean error.
Suggest the same guard gdv_fn_mask_internal already uses below: compute the bound in int64_t, compare against std::numeric_limits<int32_t>::max(), and set a context error + return nullptr when exceeded.
OK to resolve as-is if you prefer to track it separately.
DX-117112: HiveMaskConvertlets bypasses Gandiva mask functions, forcing Java execution
Why
Dremio's
HiveMaskConvertletsrewrites every SQLMASK*call intomask_internalbeforefunction resolution.
mask_internalhad no native implementation, so all string masking ran in Java and everyMASK*split reportedSplit Evaluated in Gandiva: false.Gandiva's existing
mask/mask_first_n/mask_last_n/mask_show_first_n/mask_show_last_nwere unreachable dead code.Redirecting the convertlet to those existing natives was the obvious fix and turns out to be
wrong: they do not compute the same thing as Hive. This PR makes the native functions match
Hive and adds the one signature Dremio actually emits.
Changes
1. Only
Lu,LlandNdare maskedBoth mask families now mask uppercase letters, lowercase letters and decimal digits, and pass
every other Unicode general category through — matching Hive's
GenericUDFMaskBaseandDremio's
MaskTransformer.gdv_mask_first_n_utf8_int32/gdv_mask_last_n_utf8_int32dropped an undocumentedcase 10(Nl) and now use namedUTF8PROC_CATEGORY_*enums instead of bare integers.mask_utf8_utf8_utf8_utf8droppedLt,Lo,NlandNo.This is a bug fix, not a preference. Three pieces of evidence:
passing tests:
gdv_function_stubs_test.ccassertedmask_first_n("世界您", 4)leaves CJKuntouched, while
projector_test.ccassertedmask("A的Ççd-123")masks的tox."'Masking' according to Hive specification (a-z : x, A-Z : X, 0-9 : n)" — which the code
did not implement.
docs/has nothing, and neither the registrations nor the declarationscarry comments.
spec. ARROW-14482 added the windowed family with the bare-integer switch; ARROW-17070 added
the
show_*wrappers citing Hive but delegating to it; ARROW-17121 addedmask()the nextday and deliberately expanded to
Lt/Lo/Nl/No.{1,2,9,10}is exactly the adjacentinitcaphelper's category set minusLT, which is the likely origin of the strayNl.Lois the widest-reaching: CJK, Japanese, Korean, Hebrew, Arabic, Thai, Devanagari. Casedscripts (Cyrillic, Greek, accented Latin) were already correct and are unaffected.
2. An empty replacement argument means "use the default", not "delete"
Also matching
getCharArg. This additionally fixes an under-allocation: the output buffer wassized
max(upper_len, lower_len, num_len) * data_len, which is 0 when all three argumentsare empty even though pass-through characters still get written.
SimpleArena::Allocate(0)returns the arena cursor without advancing it, so those bytes landed in space the next row's
allocation would reuse — silent cross-row corruption in a batch rather than a crash, which is
why the previous test asserted the truncated
":)"and passed.3. New:
maskwithotherChar, and nativemask_internalThe 1–4 argument
maskoverloads now delegate to a sharedmask_implwith a nullother,meaning pass-through, so their behavior is unchanged apart from change 1.
mask_internalis a single entry point for all five Hive modes (FULL,FIRST_N,LAST_N,SHOW_FIRST_N,SHOW_LAST_N) with caller-supplied replacements. It implements the fullgetCharArgcontract: an empty argument takes the per-class default, an argument parsing to-1leaves that class unmasked, and anything longer is truncated to its first character.Character counts are in codepoints.
This is what lets Dremio vectorize the shapes that actually matter. Apache Ranger's built-in
mask types generate
mask_show_last_n(col, 4, 'x', 'x', 'x', -1, '1')andmask_show_first_n(...)— custom replacement characters, which the(utf8, int32)nativeswith hardcoded
X/x/ncannot express.Behavior changes to existing functions
mask('Dž')—LtXDžmask('世'),mask('ㅏ')—Lox世,ㅏmask('Ⅷ')—NlnⅧmask('½'),mask('²')—Non½,²mask_first_n('Ⅷ', 1)—NlnⅧmask(s, '', '', '')X/x/nNo change for ASCII, or for any
Lu/Ll/Ndcharacter in any script.Four expectations in
projector_test.ccwere updated accordingly (TestMaskAll,TestMaskUpperLower,TestMaskUpper,TestMaskDefault), plus one ingdv_function_stubs_test.ccfor the empty-argument change. Those five are the entire blastradius — every other mask assertion was already Hive-compatible.
Tests
gdv_function_stubs_test.cc:TestMaskInternalModes— all five modes, empty input, unknown-mode error, case sensitivityTestMaskInternalCharCountBoundaries—INT32_MIN, negative, 0, at length, past length,INT32_MAX, across all four windowed modesTestMaskInternalReplacementArguments— custom replacements, the-1spelling and its-01variant,-2as an ordinary character, multi-character truncation, empty-means-default,and the Ranger
MASK_SHOW_LAST_4shapeTestMaskInternalUnicode— the category matrix, codepoint-counted windows, multi-bytereplacements, truncated UTF-8
TestMaskOtherChar— the new 5-argument overload through both the ASCII fast path and theutf8proc path, including
mask('王小明', …, '*') => "***"TestMaskUnicodeCategories— one case per disputed category in both families, plus across-family consistency assertion that
maskandmask_first_n(…, len)now agreetests/projector_test.cc:TestMaskInternal— end to end in the shape an engine emits: text column plus literal mode,char_countand replacements. This is what exercises the generated call, so it is the realcheck on the 15-parameter stub mapping.
TestMaskOtherChar— the 5-argument overload end to endTestMaskNullInput— closes a gap: none of the six existing mask projector tests had asingle
falsein its validity vector, sokResultNullIfNullwas untested187
gandiva-internals-testand 249gandiva-projector-testcases pass.Notes for reviewers
Known divergence from the Java implementation. Character counts are in codepoints, while
CharSequenceWrappercounts UTF-16 code units. This is visible only when achar_countboundary falls inside a surrogate pair —
mask_first_n('🌍Abc', 2)gives🌍Xbchere and🌍Abcin Java.FULLmode has nochar_countand is unaffected. Documented in the stub.Not covered.
mask_internalforint32,int64anddate. Those keep running in Java,including Ranger's
MASK_DATE_SHOW_YEAR. The int path is a handful of integer divisions andthe date path three field assignments, so the vectorization win is much smaller than for
strings; worth adding on evidence rather than speculatively.
Unrelated pre-existing issue this does not address. Because only
Lu/Ll/Ndare maskedand the
otherdefault is pass-through,mask('王小明')returns the input unchanged. Ranger'sbuilt-in "Redact" policy is plain
mask(col), so a redaction policy over uncased-script dataprovides no protection. That is Hive-compatible behavior and is tracked separately; changing it
here would have meant diverging from Hive in a masking function, which is worse than a
documented gap.
Dremio-side change required
Only the
function-list-gandiva.csvgolden, which gains two lines:No convertlet change, no new support key and no new Java functions:
mask_internalis alreadywhat the convertlet emits and already has a Java implementation, so
GandivaPushdownSievestarts pushing it down on signature match alone, and every degraded path — constant folding, a
Gandiva-less build,
exec.disabled.gandiva-functions, oversizedCASE— still falls back tothe existing Java
mask_internal.