Uh oh!
There was an error while loading. Please reload this page.
[core] Fix column masking correctness in query-auth reads - #8570
Merged
JingsongLi merged 7 commits intoAug 30, 2026
Conversation
plusplusjiajiaforce-pushed
the
core-mask-remap-fail-closed
branch
4 times, most recently
from
July 12, 2026 04:39
6bb3c6c to
117e3e5Compareplusplusjiajiaforce-pushed
the
core-mask-remap-fail-closed
branch
14 times, most recently
from
July 12, 2026 12:54
62a53ad to
5a49a17Compareplusplusjiajiaforce-pushed
the
core-mask-remap-fail-closed
branch
3 times, most recently
from
July 12, 2026 16:52
62849c7 to
17823a0CompareJingsongLi
commented
Jul 13, 2026
Contributor
|
plusplusjiajia
commented
Jul 13, 2026
MemberAuthor
@JingsongLi Agreed. I'll fold the masked-column pushdown safety into this PR so masking is correct end to end in one place, and close #8582. Updating this PR shortly. |
JingsongLi
marked this pull request as draft
July 14, 2026 13:23
plusplusjiajiaforce-pushed
the
core-mask-remap-fail-closed
branch
from
July 25, 2026 01:50
17823a0 to
fecdac2Compareplusplusjiajiaforce-pushed
the
core-mask-remap-fail-closed
branch
from
July 25, 2026 13:35
ace5904 to
a5ddf87Compareplusplusjiajia
commented
Jul 25, 2026
MemberAuthor
@JingsongLi#8582 is folded in and closed. Reviewing it turned up three more paths that reached raw values of a masked column: Please note the Behaviour changes table in the description: it rejects things that work |
plusplusjiajiaforce-pushed
the
core-mask-remap-fail-closed
branch
8 times, most recently
from
August 1, 2026 01:13
2aa1a7f to
e002e11Compareplusplusjiajiaforce-pushed
the
core-mask-remap-fail-closed
branch
from
August 7, 2026 11:46
e002e11 to
09ff9daCompareplusplusjiajiaforce-pushed
the
core-mask-remap-fail-closed
branch
3 times, most recently
from
August 26, 2026 10:36
255a441 to
fedf617Compareplusplusjiajia
marked this pull request as ready for review
August 26, 2026 11:09
A cross-column mask (e.g. display := concat_ws('-', first, last)) threw at read
time when the query projected the masked target but not the mask's inputs:
"Column masking refers to field 'first' which is not present in output row type".
Row-filter operands are already added to the read projection and projected back
out (apache#8447); do the same for column-mask inputs, transitively, and push the
widened type before planning so column pruning keeps the files they live in.
Stale rules -- columns absent from the latest schema after a rename or drop --
fail closed at plan time. The read schema is fixed once the first split reader
exists, so auth-added columns no longer leak into later splits of the same
TableRead; that leak also affected the existing row-filter path.
Scoped to query-auth.enabled tables, except the projection resets in
MergeFileSplitRead, DataEvolutionFileStoreScan and IncrementalDiffSplitRead,
which run on every table. Each fixes a case where a second withReadType left
the previous projection in place; the auth path is just the first caller that
reconfigures a read often enough to hit it.A column mask changes the value domain of its target, so a predicate on a masked column must evaluate on the masked value. Pushed into raw statistics it matches the raw value instead and prunes away the files the query should have found -- an equality filter on a masked column returned an empty result. The filter is therefore deferred to plan(): only the conjuncts free of masked columns feed statistics and partition pruning, and the masked ones are evaluated inside the auth read, post-mask. Every column they read is widened into both the scan and the read schema -- including the unmasked operands of a disjunction, which splitAnd does not split -- and projected back out. Their presence also keeps limit/TopN split pruning off. Several planning paths reached raw values without going through that: DataEvolutionBatchScan pushed straight to the SnapshotReader and consulted the global index, PrimaryKeyBatchScan kept its own unmodified filter for the sorted indexes, and partition listing skipped the check. Where a mask cannot be enforced at all, the query is refused rather than answered from raw values, since accepting it would leave the rules silently inert: - a partition predicate on a masked partition key, which pruning consumes and Spark drops from post-scan evaluation; - the system tables reporting raw per-column statistics (files, file_key_ranges, binlog); audit_log and ro read through the masking reader and still work; - vector, full-text and hybrid search, whose indexes rank raw values; - local table queries, which serve rows straight from the lookup cache; - query-auth.enabled on a table type whose read never reaches the auth reader, rejected at create time, before and after catalog table defaults apply. A mask whose transform reads another masked column is rejected too: transforms evaluate on the raw row, so it would publish that column's raw value through its own target. Masking a column with itself stays valid. Rules bind by field id, not only by name: a dropped and re-added column keeps the name but gets a fresh id, so a time-travel read would otherwise apply a rule to unrelated historical data. Scoped to query-auth.enabled tables.
Deferring the filter to the wrapped scan on a data-evolution table dropped the row-id-safe residual that `DataEvolutionBatchScan` used to pass alongside it. The wrapped scan strips masked columns but not row ids, so a `_ROW_ID` predicate reached statistics that carry logical columns only and planning threw ArrayIndexOutOfBounds -- with no masking rule configured at all. Strip the row-id part before deferring. The post-mask filter was remapped positionally against the table schema, which does not contain system fields. A masked `_ROW_ID` used in the predicate resolved to -1 and the read failed even though the field was in the emitted schema. Remap by name against that schema instead, as the rest of the auth path does.
Query auth defers the filter push to plan(), where the filter's partition conjuncts land in the same ManifestsReader slot the caller's own partition filter uses -- and overwrite it, since that slot is assigned rather than anded. ReadBuilderImpl pushes the partition filter after withFilter precisely to make it win, so the deferral silently reverses which one applies: a read combining both returned the partitions the caller had excluded. Re-apply it right after the deferred push. Off the auth path nothing is deferred, so the order stays as it was.
The scan and the read each computed the same set of columns to widen the projection by, in the same order, from the same inputs -- and they have to agree: the read schema is fixed on first use, so a scan that widens less than the read wants makes the read throw. Keep it in one place, and narrow what that leaves unused to private. Drops widenReadType, which nothing outside its own tests called. Also drops the step-by-step commentary from the tests, whose names already say what each case covers.
listPartitionEntries fetched the auth result but used only its masking rules, so a row filter on a partition key never reached pruning: the listing reported every partition, with its file and record counts, including the ones the filter excludes. plan() has always applied it; this path bypasses plan(). Four more paths served raw values with the rules in place: - t$statistics serialises the merged row count and per-column min/max, distinct and null counts, but was not rejected alongside t$files; - the table-type check ran at create only, so ALTER could turn query-auth.enabled on for a format or object table whose read ignores it. It now lives in schema validation, which both paths go through; - the search guard sat on the scan factory alone, while a pre-built plan reaches newVectorRead, newBatchVectorRead and newFullTextRead directly. The Flink and Spark subclasses override those, so they are guarded too; - a mask on _ROW_ID makes the predicate carry masked ids, which RowIdPredicateVisitor turned into a raw row range, pruning away the files the query matches. The rules are not known when the filter arrives, so the extraction is skipped whenever query auth is on. Restores the null check on DataEvolutionBatchScan's table, which the tests that exercise withFilter in isolation rely on. Two test gaps closed as well: nothing failed when DataTableStreamScan's removed second filter push was put back, and both new cases in MergeFileSplitReadTest kept every assertion inside the per-row loop, so an empty read would have passed them silently.
JingsongLi
approved these changes
Aug 30, 2026
plusplusjiajiaforce-pushed
the
core-mask-remap-fail-closed
branch
from
August 30, 2026 02:59
fedf617 to
e487930CompareUh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Makes column masking correct on the read path of
query-auth.enabledtables. Folds in #8582as asked in the first review; #8582 is closed.
Two bugs: a cross-column mask threw
Column masking refers to field 'first' which is not present in output row typewhen the query projected the masked target but not the mask'sinputs; and a predicate on a masked column was pushed into raw statistics, where it matched
the raw value rather than the masked one, returning an empty result.
Mask inputs are now widened into the read projection and projected back out, as row-filter
operands already are (#8447), transitively and preserving nested pruning. The widened type is
pushed before planning so column-file pruning keeps the files those columns live in. The read
schema is then fixed once the first split reader exists — split reads cache their format
readers, so without this the auth-added columns leaked into later splits of the same
TableRead, which also affected the pre-existing row-filter path.MergeFileSplitRead,DataEvolutionFileStoreScanandIncrementalDiffSplitReadreset stale projections onre-configuration accordingly.
The query filter is deferred to
plan(): only the conjuncts free of masked columns feedstatistics and partition pruning, and the masked ones are evaluated inside the auth read,
post-mask. Rules referencing a column absent from the latest schema fail closed at plan time
(projected system fields such as
_ROW_IDstay valid), and rules bind by field id, so adropped and re-added column of the same name cannot make a time-travel read apply a rule to
unrelated data.
Behaviour changes. Each is a case where the mask cannot be enforced, so it fails closed —
accepting the query would leave the rules silently inert, which is worse than an error.
t$files,t$file_key_ranges,t$binlogrejectedt$audit_logandt$roread through the masking reader and keep working.LocalTableQuery(lookup join) rejectedwithFilterstill works.query-auth.enabledrejected on a non-file-store tableKnown limitations. The list of bypass paths is not proven complete: the guard lives on
AbstractDataTableScan, butDataEvolutionBatchScanis a sibling andPrimaryKeyBatchScankeeps a private copy of the filter — both found by review, not by design. Reviewers who know
the global-index and data-evolution code should take a second look. Not handled here:
t$branch_xasks the catalog for its own identifier, and paimon does notenforce that it inherits the main table's rules; whether they apply is up to the catalog.
$partitions,$bucketsand$manifests, and the globalall_partitions/all_tables, report partition values straight from the manifest, so amasked partition key shows its raw value there. None of them reads through the auth reader.
Rejecting them would also block tables that enable query auth without masking a partition
key, which is the common case.