Skip to content

planner,sessionctx: apply masking in projection - #66037

Open
tiancaiamao wants to merge 13 commits into
pingcap:masterfrom
tiancaiamao:pr/phase7-8-planner
Open

tiancaiamao wants to merge 13 commits into
pingcap:masterfrom
tiancaiamao:pr/phase7-8-planner

Conversation

@tiancaiamao

@tiancaiamao tiancaiamao commented Feb 4, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: ref #65744

Problem Summary:
Column-level masking policies need to take effect at query result time ("AT RESULT" semantics). This PR implements the core projection-based masking that rewrites final query output while preserving original column values for WHERE/JOIN/GROUP BY/HAVING/ORDER BY processing.

What changed and how does it work?

Logical plan masking (planner/core):

  • buildFinalProjectionWithMasking: Builds a final LogicalProjection on top of the query plan that substitutes masked columns. Uses the child projection's expressions with ColumnSubstitute so that wrapper expressions like CONCAT(c, '-') correctly become CONCAT(mask_expr(c), '-').
  • buildFinalProjectionWithMaskingSimple: Fallback path for set operators where original SELECT fields are not available.
  • buildMaskingReplaceExprs: For each column in the schema, looks up masking policy via OrigTblName/OrigColName and builds a replacement expression.
  • Hooks into buildSelect (after LIMIT, before return) and buildSetOpr (after ORDER BY/LIMIT projection trimming) to apply masking only at the outermost level.

PointGet/BatchPointGet fast path (planner/core + executor):

  • buildMaskingExprsForPointGet: Builds masking expressions directly in the plan for single-row and batch point-get plans.
  • Executor applyMaskingExprs: Evaluates masking expressions row-by-row in PointGetExecutor.Next() and BatchPointGetExec.Next().
  • Fail-closed: If masking expressions cannot be built, newPointGetPlan/tryWhereIn2BatchPointGet return nil so the query falls back to the normal planner path (which also applies masking).

CTE handling:

  • Sets b.isCTE/b.buildingCTE flags in buildDataSourceFromCTEMerge to skip masking inside CTE definitions.
  • Preserves OrigTblName/OrigColName from the CTE source table for downstream masking policy lookup.

Set operator handling:

  • Uses buildingSetOprOperands depth counter to apply masking projection only once on the outermost set-operator result.

Expression caching (sessionctx/variable + planner/core):

  • maskingPolicyExprCache: Caches parsed masking expressions in SessionVars, keyed by schema version.
  • getMaskingPolicyExpr: Parses the masking expression string, caches the result, with a recursion guard to prevent infinite loops.
  • Cache is invalidated automatically when schema version changes.

Tests:

make failpoint-enable && (cd pkg/planner/core && go test -tags intest -run "TestMaskingPolicyExprCache|TestMaskingPolicyProjection|TestMaskingPolicyAtResult" -v -count=1; rc=$?; cd - >/dev/null; make failpoint-disable; exit $rc)

Test coverage:

  • TestMaskingPolicyExprCache / TestMaskingPolicyExprCacheRecursion: Cache hit/miss, schema version invalidation, recursion detection.
  • TestMaskingPolicyProjection: Basic projection masking, PointGet masking, BatchPointGet masking (primary key IN, unique index IN, expression wrapping).
  • TestMaskingPolicyAtResult: HAVING uses original values, ORDER BY uses original values, UNION DISTINCT deduplication uses original values, aggregate with HAVING uses original values.

Stacked PRs

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Summary by CodeRabbit

  • New Features

    • "AT RESULT" masking: result columns are masked for output while HAVING/ORDER BY/UNION use original values so filtering/sorting/deduplication remain correct.
    • Masking is enforced in fast lookup paths (point-get / batch point-get) with a fail-closed fallback and per-session caching of compiled masking expressions.
  • Tests

    • Added comprehensive tests covering at-result masking semantics, fast-path behavior, set-ops, projection cases, and cache/recursion scenarios.

@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/planner SIG: Planner labels Feb 4, 2026
@tiprow

tiprow Bot commented Feb 4, 2026

Copy link
Copy Markdown

Hi @tiancaiamao. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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.

@tiancaiamao tiancaiamao mentioned this pull request Feb 4, 2026
2 of 13 tasks
Implements the core masking policy projection feature:

- Logical plan: buildFinalProjectionWithMasking applies masking as a final
  projection on top of SELECT/UNION results, preserving original values for
  WHERE/JOIN/GROUP BY/HAVING/ORDER BY processing (AT RESULT semantics).
- PointGet/BatchPointGet fast path: builds MaskingExprs directly in the plan
  and applies them in the executor after row decoding.
- CTE handling: sets isCTE/buildingCTE flags to skip masking inside CTE
  definitions, preserving OrigTblName/OrigColName for downstream lookup.
- Expression cache: caches parsed masking expressions in SessionVars keyed
  by schema version with recursion guard.
- Set operator handling: uses buildingSetOprOperands depth counter to apply
  masking only at the outermost set-operator result.
- Tests: TestMaskingPolicyExprCache, TestMaskingPolicyProjection,
  TestMaskingPolicyAtResult (HAVING/ORDER BY/UNION/Aggregate coverage).
@tiancaiamao
tiancaiamao force-pushed the pr/phase7-8-planner branch from 9fd16d1 to 5f315b4 Compare June 9, 2026 14:55
@coderabbitai

coderabbitai Bot commented Jun 9, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds AT RESULT column masking: session-scoped expression cache, planner helpers to build per-column masking expressions, MaskingExprs on point-get/batch-point-get plans, executor masking application after result production, and tests validating semantics.

Changes

AT RESULT Column Masking

Layer / File(s) Summary
Session caching and masking expression infrastructure
pkg/sessionctx/variable/session.go, pkg/sessionctx/variable/masking_policy_cache.go, pkg/planner/core/masking_policy_expr_cache.go, pkg/planner/core/BUILD.bazel, pkg/sessionctx/variable/BUILD.bazel
Adds SessionVars fields and getter/setter for a masking-policy expression cache keyed by schema version; implements per-policy cached parsed expressions with recursion detection and rebuild-on-schema-change.
Logical plan builder masking support
pkg/planner/core/logical_plan_builder.go, pkg/planner/core/planbuilder.go
Adds helpers to build masking replacement expressions, per-plan-node caching, applyMasking support in projections, set-operator nesting tracking, CTE name preservation, and a final-stage masking projection applied after ORDER BY/LIMIT.
Point-get fast-path masking expression construction
pkg/planner/core/point_get_plan.go
Builds per-output-column masking expressions for point-get and batch-point-get fast paths by resolving masking policies via original column identifiers; fail-closes the fast path on builder errors.
Physical plan and executor masking integration
pkg/planner/core/operator/physicalop/physical_batch_point_get.go, pkg/executor/point_get.go, pkg/executor/batch_point_get.go, pkg/executor/builder.go
Adds MaskingExprs []expression.Expression on point-get/batch-point-get plans; executors copy these fields and apply masking to output chunks after virtual column computation using applyMaskingExprs.
Integration and unit tests
pkg/planner/core/masking_policy_expr_cache_test.go, pkg/planner/core/masking_policy_projection_test.go, pkg/planner/core/masking_policy_at_result_test.go
Tests cover expression-cache reuse and recursion, masked projection behavior, point-get/batch-point-get masking, and AT RESULT semantics for HAVING, ORDER BY, and UNION DISTINCT deduplication.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant PlanBuilder
  participant SessionVars
  participant PointGetPlan
  participant Executor
  participant Output
  Client->>PlanBuilder: buildSelect / point-get plan
  PlanBuilder->>SessionVars: GetMaskingPolicyExprCache()
  SessionVars->>PlanBuilder: return cache (or trigger build)
  PlanBuilder->>PointGetPlan: attach MaskingExprs
  Client->>Executor: execute PointGetExec / BatchPointGetExec
  Executor->>Executor: produce chunk, fill virtual cols
  alt MaskingExprs present and rows exist
    Executor->>Executor: applyMaskingExprs(chunk, MaskingExprs)
  end
  Executor->>Output: return masked chunk
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested labels

ok-to-test

Suggested reviewers

  • wjhuang2016
  • yudongusa
  • AilinKid
  • qw4990

Poem

🐰 I nibble on queries, soft and spry,
I weave a mask beneath the sky.
Planner builds, the cache keeps score,
Executors paint the rows once more.
Hoppity—masked results hop out the door!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'planner,sessionctx: apply masking in projection' clearly and concisely describes the main change—applying masking at projection time in the planner and session context modules.
Description check ✅ Passed The PR description is comprehensive and follows the template with all required sections: problem statement (Issue ref #65744), detailed explanation of changes, checklist completion, and test coverage information.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (3)
pkg/executor/point_get.go (1)

456-490: ⚡ Quick win

Validate masking-expression arity before chunk rewrite.

applyMaskingExprs assumes len(maskingExprs) == len(schema.Columns). If this contract is ever violated upstream, chunk columns can become inconsistent (or panic). Add an upfront length check and return a contextual error.

Proposed patch
diff --git a/pkg/executor/point_get.go b/pkg/executor/point_get.go
@@
 func applyMaskingExprs(sctx sessionctx.Context, schema *expression.Schema, maskingExprs []expression.Expression, req *chunk.Chunk) error {
+	if len(maskingExprs) != len(schema.Columns) {
+		return errors.Errorf("masking expr count mismatch: got %d, expected %d", len(maskingExprs), len(schema.Columns))
+	}
 	fieldTypes := make([]*types.FieldType, len(schema.Columns))
🤖 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 `@pkg/executor/point_get.go` around lines 456 - 490, applyMaskingExprs
currently assumes len(maskingExprs) == len(schema.Columns) which can corrupt
chunks; add an upfront check in applyMaskingExprs comparing len(maskingExprs) to
len(schema.Columns) and return a clear error (including lengths and context) if
they differ so the function fails fast instead of producing an inconsistent
maskedChunk or panicking; reference the maskingExprs slice, schema.Columns, and
the applyMaskingExprs function in the error message to aid debugging.
pkg/planner/core/point_get_plan.go (1)

1492-1494: 💤 Low value

Unused ctx context.Context parameter.

The ctx parameter is declared but never used in the function body. Both call sites currently pass context.Background(). Either use the context (e.g., for cancellation/deadline propagation in future operations) or remove the parameter to avoid confusion.

🔧 Suggested fix if keeping the parameter is not needed
 func buildMaskingExprsForPointGet(
-	ctx context.Context,
 	sctx base.PlanContext,
 	is infoschema.InfoSchema,
 	schema *expression.Schema,
 	names []*types.FieldName,
 	tbl *model.TableInfo,
 ) ([]expression.Expression, error) {

And update call sites:

-maskExprs, err := buildMaskingExprsForPointGet(context.Background(), ctx, is.(infoschema.InfoSchema), schema, names, tbl)
+maskExprs, err := buildMaskingExprsForPointGet(ctx, is.(infoschema.InfoSchema), schema, names, tbl)
🤖 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 `@pkg/planner/core/point_get_plan.go` around lines 1492 - 1494, The function
buildMaskingExprsForPointGet currently declares an unused ctx parameter; remove
the ctx context.Context parameter from its signature and update all call sites
that pass context.Background() to no longer pass that argument (search for
buildMaskingExprsForPointGet usages). Alternatively, if you intend to use a
context for cancellation later, thread the existing ctx into any internal calls
where context propagation is appropriate (and stop passing context.Background()
from callers) — choose one approach and make signatures consistent across the
function and its callers.
pkg/planner/core/masking_policy_expr_cache_test.go (1)

76-103: ⚡ Quick win

Verify the specific error message in recursion test.

The test correctly simulates recursion by setting cache.building[policy.ID], but only checks that an error is returned without validating its content. For stronger test coverage, verify the expected error message matches "masking policy expression recursion detected" from the implementation.

✅ Suggested enhancement
 	cache := getMaskingPolicyExprCache(sv, 1)
 	cache.building[policy.ID] = struct{}{}
 	_, _, err := getMaskingPolicyExpr(ctx, sv, 1, policy, tblInfo, colInfo)
 	require.Error(t, err)
+	require.Contains(t, err.Error(), "masking policy expression recursion detected")
🤖 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 `@pkg/planner/core/masking_policy_expr_cache_test.go` around lines 76 - 103,
Update the TestMaskingPolicyExprCacheRecursion test to assert the specific
recursion error message: after setting cache.building[policy.ID] and calling
getMaskingPolicyExpr(ctx, sv, 1, policy, tblInfo, colInfo), verify err is not
nil and that err.Error() (or errors.Is/As if using wrapped errors) contains or
equals "masking policy expression recursion detected" so the test validates the
exact failure from getMaskingPolicyExpr and not just any error.
🤖 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/executor/point_get.go`:
- Line 98: The executor's MaskingExprs field is only set in buildPointGet which
leaves recycled executors with stale masking rules; update the reusable
initializer Init(p) (used by Recreated) to set e.MaskingExprs = p.MaskingExprs
(and clear/reset it when p.MaskingExprs is nil) so any reused executor always
reflects the incoming plan's masking expressions; apply the same change to the
other build paths referenced (the alternative build functions that currently
only assign MaskingExprs in their builder code) so all Init-based reuse paths
initialize MaskingExprs consistently.

In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 5182-5187: The non-merge path in adjustCTEPlanOutputName
overwrites outPutNames[i].ColName with the CTE column-list alias without
preserving outPutNames[i].OrigColName, causing inconsistent masking between
merge and non-merge plans; update adjustCTEPlanOutputName so that before
assigning the alias to outPutNames[i].ColName it checks if
outPutNames[i].OrigColName.L is empty and if so sets outPutNames[i].OrigColName
= outPutNames[i].ColName, then assign outPutNames[i].ColName =
cte.ColNameList[i] (mirroring the merge-path logic), ensuring findMaskingPolicy
still sees the original source name.
- Around line 2423-2430: The final masking call always passes nil to
buildFinalProjectionWithMasking when building set-operator results (in the block
guarded by b.buildingSetOprOperands/isCTE/buildingCTE), which forces the
fallback to name-only resolution and loses DBName/OrigTblName/OrigColName;
ensure the original column identity is preserved by either (A) updating
buildProjection4Union to copy DBName/OrigTblName/OrigColName into the outer
target columns (not just ColName) so masking can find policies later, or (B)
change the call to buildFinalProjectionWithMasking(setOprPlan, oldLen,
sourceCols) and pass the original source column metadata produced by
buildProjection4Union so buildFinalProjectionWithMasking can use those
identities instead of the nil/name-only path; locate symbols
buildFinalProjectionWithMasking, buildProjection4Union, buildingSetOprOperands,
and ColName to implement the fix.
- Around line 1596-1600: The current code in logical_plan_builder.go uses
name.DBName fallback to session CurrentDB before calling b.is.TableByName, which
causes derived/aliased outputs (whose FieldName.DBName was intentionally
cleared) to be resolved against CurrentDB and break masking; change the logic so
that when name.DBName is empty you do NOT substitute
b.ctx.GetSessionVars().CurrentDB for derived fields — instead preserve an empty
DBName (or only apply the CurrentDB fallback when you can prove the field came
from a base table), and avoid calling b.is.TableByName with the substituted
CurrentDB for those derived outputs; adjust the dbName/docker variable handling
around the dbName := name.DBName and the b.is.TableByName call accordingly.

In `@pkg/planner/core/operator/physicalop/physical_batch_point_get.go`:
- Around line 87-90: PointGetPlan.MemoryUsage() and
BatchPointGetPlan.MemoryUsage() currently omit the MaskingExprs slice,
undercounting plan memory; update both MemoryUsage methods to include the memory
used by MaskingExprs by iterating over the MaskingExprs slice and adding each
expression's memory cost using the same expression memory-accounting helper
already used elsewhere (i.e., the existing expression memory-usage helper used
in these methods), so the total includes the slice overhead plus per-expression
memory.

In `@pkg/planner/core/point_get_plan.go`:
- Around line 776-779: The code incorrectly infers DML synthetic SELECTs by
testing len(names)>0 and thus attaches MaskingExprs for UPDATE/DELETE point-get
fast paths; change the logic so masking is skipped for DML plans by making the
DML intent explicit: update newPointGetPlan to accept an isDML (or skipMasking)
boolean and stop relying on len(names) to detect DML, then modify callers
tryUpdatePointPlan and tryDeletePointPlan to pass isDML=true (or set
skipMasking) when they create the SelectStmt/point-get plan; alternatively, if
you prefer to keep the single-signature, detect DML by inspecting the original
statement type passed into newPointGetPlan (e.g., the AST node representing
UPDATE/DELETE) and skip building MaskingExprs when it is a DML, ensuring
PointGetExec and BatchPointGetExec never receive MaskingExprs for DML branches
so UpdateExec consumes unmasked column values.

---

Nitpick comments:
In `@pkg/executor/point_get.go`:
- Around line 456-490: applyMaskingExprs currently assumes len(maskingExprs) ==
len(schema.Columns) which can corrupt chunks; add an upfront check in
applyMaskingExprs comparing len(maskingExprs) to len(schema.Columns) and return
a clear error (including lengths and context) if they differ so the function
fails fast instead of producing an inconsistent maskedChunk or panicking;
reference the maskingExprs slice, schema.Columns, and the applyMaskingExprs
function in the error message to aid debugging.

In `@pkg/planner/core/masking_policy_expr_cache_test.go`:
- Around line 76-103: Update the TestMaskingPolicyExprCacheRecursion test to
assert the specific recursion error message: after setting
cache.building[policy.ID] and calling getMaskingPolicyExpr(ctx, sv, 1, policy,
tblInfo, colInfo), verify err is not nil and that err.Error() (or errors.Is/As
if using wrapped errors) contains or equals "masking policy expression recursion
detected" so the test validates the exact failure from getMaskingPolicyExpr and
not just any error.

In `@pkg/planner/core/point_get_plan.go`:
- Around line 1492-1494: The function buildMaskingExprsForPointGet currently
declares an unused ctx parameter; remove the ctx context.Context parameter from
its signature and update all call sites that pass context.Background() to no
longer pass that argument (search for buildMaskingExprsForPointGet usages).
Alternatively, if you intend to use a context for cancellation later, thread the
existing ctx into any internal calls where context propagation is appropriate
(and stop passing context.Background() from callers) — choose one approach and
make signatures consistent across the function and its callers.
🪄 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: f2a0c117-7d63-4059-acd0-8f13b9c1ff5c

📥 Commits

Reviewing files that changed from the base of the PR and between 52ab330 and 5f315b4.

📒 Files selected for processing (13)
  • pkg/executor/batch_point_get.go
  • pkg/executor/builder.go
  • pkg/executor/point_get.go
  • pkg/planner/core/logical_plan_builder.go
  • pkg/planner/core/masking_policy_at_result_test.go
  • pkg/planner/core/masking_policy_expr_cache.go
  • pkg/planner/core/masking_policy_expr_cache_test.go
  • pkg/planner/core/masking_policy_projection_test.go
  • pkg/planner/core/operator/physicalop/physical_batch_point_get.go
  • pkg/planner/core/planbuilder.go
  • pkg/planner/core/point_get_plan.go
  • pkg/sessionctx/variable/masking_policy_cache.go
  • pkg/sessionctx/variable/session.go

Comment thread pkg/executor/point_get.go Outdated
Comment thread pkg/planner/core/logical_plan_builder.go
Comment thread pkg/planner/core/logical_plan_builder.go Outdated
Comment thread pkg/planner/core/logical_plan_builder.go
Comment thread pkg/planner/core/operator/physicalop/physical_batch_point_get.go
Comment thread pkg/planner/core/point_get_plan.go Outdated
1. DML masking in PointGet fast path: moved masking out of newPointGetPlan
   into tryPointGetPlan via maybeApplyPointGetMasking helper, which checks
   len(selStmt.Fields.Fields) > 0 to skip DML synthetic SELECTs.
   Similarly fixed tryWhereIn2BatchPointGet.

2. MaskingExprs in Recreated/Init: moved assignment from buildPointGet into
   Init(p) so Recreated path also refreshes the field.

3. findMaskingPolicy CurrentDB fallback: when DBName is empty, return nil
   instead of guessing via CurrentDB (avoids wrong-table masking).

4. buildProjection4Union OrigTblName/OrigColName: preserve all FieldName
   fields from first child, not just ColName.

5. adjustCTEPlanOutputName OrigColName: don't overwrite OrigColName with
   CTE alias when it was already set by the CTE merge path.

6. applyMaskingExprs arity check: add defensive length validation.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pkg/planner/core/masking_policy_expr_cache_test.go (1)

101-102: 💤 Low value

Consider checking the specific error message or type.

The recursion detection test validates that an error is returned but doesn't verify it's the expected recursion error. Checking the error message or using require.ErrorContains would make the test more robust against future changes.

♻️ Optional improvement
 _, _, err := getMaskingPolicyExpr(ctx, sv, 1, policy, tblInfo, colInfo)
-require.Error(t, err)
+require.ErrorContains(t, err, "recursion")  // or check for specific error type
🤖 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 `@pkg/planner/core/masking_policy_expr_cache_test.go` around lines 101 - 102,
The test currently only asserts an error is returned from getMaskingPolicyExpr;
change the assertion to verify it's the expected recursion error by either using
require.ErrorContains(t, err, "recursion") with a suitable substring (e.g.,
"recursion" or the exact message produced) or, if a sentinel error exists, use
require.True(t, errors.Is(err, ErrMaskingRecursion)) to assert the error type;
update the assertion replacing require.Error(t, err) accordingly to make the
test robust.
🤖 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.

Nitpick comments:
In `@pkg/planner/core/masking_policy_expr_cache_test.go`:
- Around line 101-102: The test currently only asserts an error is returned from
getMaskingPolicyExpr; change the assertion to verify it's the expected recursion
error by either using require.ErrorContains(t, err, "recursion") with a suitable
substring (e.g., "recursion" or the exact message produced) or, if a sentinel
error exists, use require.True(t, errors.Is(err, ErrMaskingRecursion)) to assert
the error type; update the assertion replacing require.Error(t, err) accordingly
to make the test robust.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 6a77372d-82bc-40fd-9b63-1adc37c844ba

📥 Commits

Reviewing files that changed from the base of the PR and between a609cef and 2ca8389.

📒 Files selected for processing (4)
  • pkg/planner/core/BUILD.bazel
  • pkg/planner/core/masking_policy_expr_cache.go
  • pkg/planner/core/masking_policy_expr_cache_test.go
  • pkg/sessionctx/variable/BUILD.bazel
✅ Files skipped from review due to trivial changes (2)
  • pkg/sessionctx/variable/BUILD.bazel
  • pkg/planner/core/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/planner/core/masking_policy_expr_cache.go

The buildingCTE flag has a specific meaning for recursive CTE tracking —
it requires b.outerCTEs to be non-empty. Setting it in
buildDataSourceFromCTEMerge (for non-recursive CTEs) caused panics when
buildDistinct/buildAgg accessed b.outerCTEs[-1].

Fix: only set b.isCTE=true in buildDataSourceFromCTEMerge, not
b.buildingCTE. The isCTE flag is sufficient for masking guard checks.

Also fixes test ordering flakiness in TestMaskingPolicyAtResult by adding
ORDER BY clause to non-deterministic GROUP BY query.
@codecov

codecov Bot commented Jun 10, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 20.40816% with 351 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.3014%. Comparing base (d42b17d) to head (0fc9cf0).
⚠️ Report is 284 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #66037        +/-   ##
================================================
- Coverage   76.3233%   76.3014%   -0.0219%     
================================================
  Files          2041       2061        +20     
  Lines        562646     581824     +19178     
================================================
+ Hits         429430     443940     +14510     
- Misses       132305     135475      +3170     
- Partials        911       2409      +1498     
Flag Coverage Δ
integration 46.0580% <20.4081%> (+6.3366%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 60.4610% <ø> (ø)
parser ∅ <ø> (∅)
br 65.0960% <ø> (+2.3052%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

1. buildMaskingReplaceExprs: when getMaskingPolicyExpr fails (e.g.,
   DDL rename column transition where expression still references old
   column name), log warning and skip instead of failing the query.

2. findMaskingPolicy: restored CurrentDB fallback for empty DBName to
   match reference implementation behavior.
@tiancaiamao

Copy link
Copy Markdown
Contributor Author

/retest

@tiprow

tiprow Bot commented Jun 10, 2026

Copy link
Copy Markdown

@tiancaiamao: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

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.

After TRUNCATE TABLE, infoSchema may have stale masking policy data
because loadMaskingPoliciesIfNeeded uses a snapshot TS that predates
the DDL worker's UPDATE to mysql.tidb_masking_policy.

Add a fallback: when infoSchema lookup fails, query the system table
directly via ExecRestrictedSQL with snapshotTS=0 (latest data).

Renames getMaskingPolicyByNameForTable to getMaskingPolicyByNameForDDL
to match the reference implementation.
The new MaskingExprs field on PointGetPlan was not being copied during
fast clone, causing TestFastPointGetClone to fail.
@tiancaiamao

Copy link
Copy Markdown
Contributor Author

/retest

@tiprow

tiprow Bot commented Jun 11, 2026

Copy link
Copy Markdown

@tiancaiamao: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

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.

// For CTEs, we skip masking here because:
// 1. CTE definitions should preserve original values for correct filtering/joining
// 2. Masking is applied when CTE results are materialized to the final output
if b.buildingSetOprOperands == 0 && !b.isCTE && !b.buildingCTE {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also runs inside derived tables. Outer filters may then see masked values, not original values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added a buildingResultSetNode flag that is set in buildResultSetNode and checked in both buildSelect and buildSetOpr to skip masking inside derived tables. Masking is only applied at the outermost query level, so outer filters/joins always see original values.

OrigColName: firstChildNames[i].OrigColName,
ColName: firstChildNames[i].ColName,
})
unionCols = append(unionCols, &expression.Column{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please preserve the source column ID here; otherwise UNION output cannot find the masking policy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added ID: col.ID to the union column construction so findMaskingPolicy can match the column ID correctly.

if policy.Status != model.MaskingPolicyStatusEnable {
continue
}
expr, placeholder, err := getMaskingPolicyExpr(b.ctx.GetExprCtx(), sv, schemaVersion, policy, tblInfo, colInfo)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please return an error here. Skipping masking can leak raw column values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Changed to return nil, errors.Trace(err) so the query fails closed rather than silently leaking the raw column value.

// even though they mask to the same pattern
result := tk.MustQuery("select val from t_union where id = 1 union distinct select val from t_union where id = 2")
rows := result.Rows()
require.Len(t, rows, 2)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also check the returned values are masked; row count alone misses raw output.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Changed to result.Sort().Check(testkit.Rows("[value1]", "[value2]")) to verify the actual masked output values, not just the row count.

@tiancaiamao

Copy link
Copy Markdown
Contributor Author

/retest

@tiprow

tiprow Bot commented Jun 16, 2026

Copy link
Copy Markdown

@tiancaiamao: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

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 expxiaoli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot

ti-chi-bot Bot commented Jun 16, 2026

Copy link
Copy Markdown

@expxiaoli: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

LGTM

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.

@tiancaiamao

Copy link
Copy Markdown
Contributor Author

/retest

@expxiaoli expxiaoli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot

ti-chi-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: expxiaoli
Once this PR has been reviewed and has the lgtm label, please assign d3hunter, terry1purcell for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 17, 2026
@ti-chi-bot

ti-chi-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-08-17 06:40:43.203878356 +0000 UTC m=+3633429.239973412: ☑️ agreed by expxiaoli.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-1-more-lgtm Indicates a PR needs 1 more LGTM. release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants