Repository navigation
expression: add column masking related builtins functions - #68321
tiancaiamao wants to merge 14 commits into
Conversation
ti-chi-bot
Bot
added
the
release-note
|
@tiancaiamao I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/XXL
|
Hi @tiancaiamao. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds four masking builtins—mask_full, mask_null, mask_partial, mask_date—with implementations, signature selection, collation derivation updates, unit and integration tests, build/registry wiring, and generated thread-safety wrappers. ChangesMasking Builtins Feature
Sequence Diagram(s)sequenceDiagram
participant SQL as Client
participant Eval as expression.Evaluator
participant Factory as mask_* FunctionClass
participant Sig as mask_* signature
SQL->>Eval: call mask_full/mask_null/mask_partial/mask_date(args)
Eval->>Factory: getFunction(arg types/count)
Factory->>Sig: select signature variant (string/binary/time/num)
Sig->>Eval: eval -> masked value / NULL / parsed date
Eval->>SQL: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/integrationtest/t/expression/builtin.test (1)
1676-1692: ⚡ Quick winConsider adding test coverage for optional mask_char parameter.
The test coverage for the masking functions is comprehensive, but line 1677 tests
mask_full()only with an explicit mask character. Since the PR description indicatesmask_charis optional (MASK_FULL(col[, mask_char])), consider adding a test case that exercises the default behavior:select mask_full('abc'), mask_full(cast('2012-01-02' as date));This would verify that the default mask character works correctly across different types.
Important: Use the correct recording command for integration tests.
As per coding guidelines, when recording the expected results for these new test cases, you must use the recording command documented in
docs/agents/testing-flow.md→Integration tests(not-record).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integrationtest/t/expression/builtin.test` around lines 1676 - 1692, Add a test case that calls mask_full without the optional mask_char to verify default behavior (e.g., select mask_full('abc'), mask_full(cast('2012-01-02' as date))) and add similar assertions for other types already exercised; ensure you record the expected results using the documented integration test recording command from the testing-flow.md (not the -record flag).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/expression/builtin_masking_test.go`:
- Around line 107-136: Extend TestMaskPartial to include a case that uses very
large preserve values (e.g., math.MaxInt64 and math.MaxInt64-1) when
constructing the function via newFunctionForTest with ast.MaskPartial and
primitiveValsToConstants; call f.Eval(ctx, chunk.Row{}) inside require.NotPanics
(and then require.NoError if you expect a normal return) to assert the call does
not panic for extreme preserve_left/preserve_right inputs, referencing
TestMaskPartial, newFunctionForTest, ast.MaskPartial, primitiveValsToConstants,
and chunk.Row{} to locate where to add the case.
In `@pkg/expression/builtin_masking.go`:
- Around line 438-451: The preserve-range check in both
builtinMaskPartialSig.evalString and builtinMaskPartialUTF8Sig.evalString can
overflow when adding preserveLeft and preserveRight (int64) and then panic
during slicing; update both functions to perform overflow-safe checks before
summing (e.g., validate each preserve value against total and use a
subtraction-based comparison like preserveLeft >= total || preserveRight >=
total || preserveLeft > total - preserveRight) so the guard correctly returns
the original string when the preserved ranges cover or exceed total and prevents
computing maskLen or slicing on overflowed values.
- Around line 217-222: The return type for mask_null must never carry the
NotNullFlag; update the mask_null builtin to clone the input field type and
explicitly clear the NotNullFlag before assigning it to bf.tp (i.e., after bf.tp
= argType.Clone(), clear the NotNullFlag on bf.tp). Reference the mask_null
implementation and the bf.tp assignment in builtin_masking.go to locate where to
perform the flag removal.
---
Nitpick comments:
In `@tests/integrationtest/t/expression/builtin.test`:
- Around line 1676-1692: Add a test case that calls mask_full without the
optional mask_char to verify default behavior (e.g., select mask_full('abc'),
mask_full(cast('2012-01-02' as date))) and add similar assertions for other
types already exercised; ensure you record the expected results using the
documented integration test recording command from the testing-flow.md (not the
-record flag).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb23ecfa-d456-47b4-93c4-293097625e14
📒 Files selected for processing (7)
pkg/expression/BUILD.bazelpkg/expression/builtin.gopkg/expression/builtin_masking.gopkg/expression/builtin_masking_test.gopkg/expression/collation.gotests/integrationtest/r/expression/builtin.resulttests/integrationtest/t/expression/builtin.test
|
/retest-required |
|
@tiancaiamao: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@tiancaiamao: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68321 +/- ##
================================================
- Coverage 77.3057% 73.9489% -3.3568%
================================================
Files 2010 2044 +34
Lines 555533 602621 +47088
================================================
+ Hits 429459 445632 +16173
- Misses 125154 154421 +29267
- Partials 920 2568 +1648
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…master # Conflicts: # pkg/expression/function_traits_test.go # pkg/planner/util/null_misc_test.go # tests/integrationtest/r/executor/show.result
tiancaiamao
changed the title
tiancaiamao
changed the title
| %Y-%m-%d %Y-%m-%d %H:%i:%s %Y-%m-%d %H.%i.%s %H%i%s | ||
| select mask_full('abc', '*'), mask_full(cast('2012-01-02' as date), '*'), mask_full(cast('2012-01-02 03:04:05' as datetime), '*'), mask_full(cast('03:04:05' as time), '*'), mask_full(cast('2012' as year), '*'); | ||
| mask_full('abc', '*') mask_full(cast('2012-01-02' as date), '*') mask_full(cast('2012-01-02 03:04:05' as datetime), '*') mask_full(cast('03:04:05' as time), '*') mask_full(cast('2012' as year), '*') | ||
| *** 1970-01-01 1970-01-01 00:00:00 00:00:00 0000 |
There was a problem hiding this comment.
mask_full(cast('2012-01-02' as date), '*') -> 1970-01-01
Is this expected? if yes, it's better to documented it in the user doc('*' will be ignored).
There was a problem hiding this comment.
Fixed ... according to design, mask_full only accept one parameter.
mask_full should only take a single argument (the value to mask), not an optional second mask character argument. Remove the second argument handling from getFunction, evalString methods, unit tests, and integration tests. Addresses PR review comment: mask_full function signature should match the design spec at docs/design/2026-02-27-column-level-masking.md.
| } | ||
| return &builtinMaskFullIntSig{bf}, nil | ||
| default: | ||
| return nil, errIncorrectArgs.GenWithStackByArgs("mask_full") |
There was a problem hiding this comment.
Do you plan to support DECIMAL and DOUBLE type here?
There was a problem hiding this comment.
Updated, now DECIMAL and DOUBLE are supported.
|
/retest |
|
@tiancaiamao: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@expxiaoli: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| select mask_full('abc', '*'), mask_full(cast('2012-01-02' as date), '*'), mask_full(cast('2012-01-02 03:04:05' as datetime), '*'), mask_full(cast('03:04:05' as time), '*'), mask_full(cast('2012' as year), '*'); | ||
| mask_full('abc', '*') mask_full(cast('2012-01-02' as date), '*') mask_full(cast('2012-01-02 03:04:05' as datetime), '*') mask_full(cast('03:04:05' as time), '*') mask_full(cast('2012' as year), '*') | ||
| *** 1970-01-01 1970-01-01 00:00:00 00:00:00 0000 | ||
| select mask_full('abc'), mask_full(cast('2012-01-02' as date)), mask_full(cast('2012-01-02 03:04:05' as datetime)), mask_full(cast('03:04:05' as time)), mask_full(cast('2012' as year)); |
There was a problem hiding this comment.
Do we have plan to support format like mask_partial(birth_date, 'Y-01-01')? There was something like it in the specification doc.
There was a problem hiding this comment.
You mean mask_date, not mask_partial...
I've updated the code to support that.
| return nil, err | ||
| } | ||
| argType := args[0].GetType(ctx.GetEvalCtx()) | ||
| bf.tp = argType.Clone() |
There was a problem hiding this comment.
What if args[0] is of non-string types?
|
PTAL @bb7133 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: expxiaoli The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
This branch has not been deployed
3 participants
What problem does this PR solve?
Issue Number: ref #65744, close #67224
Problem Summary:
pr/phase9-builtinswas stacked on top of other masking phases, making direct merge/rebase to latestmasterconflict-prone.This PR ports only the masking builtins phase onto current
masterand aligns behavior with the implementation inissue-bundle-67218-67224.What changed and how does it work?
MASK_FULL(col[, mask_char])MASK_NULL(col)MASK_PARTIAL(col, preserve_left, preserve_right, mask_char)MASK_DATE(col, date_literal)tests/integrationtest/t/expression/builtin.testand expected results.Check List
Tests
Test commands:
GOTOOLCHAIN=go1.25.9 ./tools/check/failpoint-go-test.sh pkg/expression -run 'TestMask(Full|Null|Partial|Date)$'cd tests/integrationtest && ./run-tests.sh -r expression/builtinSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Behavior
Tests