[fix](policy) Enforce row policies on MOW time travel - #67783
Merged
morrySnow merged 5 commits intoSep 17, 2026
Merged
Conversation
morrySnow
requested review from
924060929,
englefly and
starocean999
as code owners
September 10, 2026 07:54
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
morrySnow
marked this pull request as draft
September 11, 2026 05:29
morrySnow
force-pushed
the
fix/mow-time-travel-row-policy
branch
from
September 11, 2026 06:41
9f36362 to
0aedb2b
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16732 ms |
Contributor
TPC-DS: Total hot run time: 81249 ms |
Contributor
ClickBench: Total hot run time: 14.75 s |
morrySnow
marked this pull request as ready for review
September 12, 2026 08:18
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes because the new MOW time-travel policy placement leaves two authorization failures.
Critical checkpoint conclusions:
- Goal/correctness/security: native row filters are exercised on both reconstructed branches, but ordinary plugin data masks can be eliminated and expose raw historical values. The two branches also resolve live policy state independently, so a policy transition can produce a mixed result whose SQL-cache proof records only the later policy. Both affect an in-scope restricted SQL user.
- Scope/parallel paths: recursive
OlapTableWrapperunwrapping is correct for current wrapper schemas. DUP time travel, direct@incr, ordinary scans, MV refresh, no-policy, and root/admin paths were traced without another distinct defect. - Concurrency/lifecycle/configuration/compatibility: the policy-DDL/plugin-refresh interleaving is covered by the inline cache finding. No separate lifecycle, configuration, persistence/edit-log, FE-BE protocol, storage-format, or rolling-upgrade issue is introduced.
- Tests: the new regression covers native restrictive/permissive row filters across base and binlog branches, but it has no data-mask case and does not follow the required generated-oracle/post-test-state rules. The author reports unit, regression, and checkstyle results; this review is static-only and did not independently run builds or tests, as required by the review environment.
- Performance/observability: duplicate provider work shares the mixed-policy root cause and remedy; no separate performance or observability blocker was substantiated.
No additional user review focus was provided. The review converged after two rounds, with all round-two reviewers returning NO_NEW_VALUABLE_FINDINGS beyond the three inline comments.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#67783 Problem Summary: MOW time-travel branch projections captured raw scan slots before row policies and data masks were expanded. Later project merging could bypass the mask in either the base-scan or row-binlog branch. The regression also asserted results inline and deleted its state instead of using the repository oracle and cleanup contract. Build visible branch outputs as unbound slots so they bind to post-policy outputs, cover both branches, and use a generated regression oracle while retaining the final test state. ### Release note MOW time-travel queries now preserve data masking in both union branches. ### Check List (For Author) - Test: - Unit Test: CheckRowPolicyTest (11 tests) - Regression test: test_mow_time_travel_row_policy - FE build with Checkstyle - Behavior changed: Yes (masked values remain masked for MOW time-travel queries) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes for one remaining MOW time-travel data-mask ordering defect.
Critical checkpoint conclusions:
- Goal/data correctness/security: the change now enforces native row policies on both reconstruction sources and preserves ordinary movable visible-column masks, but it does not safely handle every supported data mask because reconstruction filters can remain above the mask project. This affects an in-scope restricted SQL user.
- Scope/clarity and parallel paths: the five-file change is focused. Recursive wrapper unwrapping, MV-refresh identity, union output order/types and qualifiers, and ordinary, DUP, direct
@incr, partition/tablet/index, hint, and sample paths were traced without another distinct defect. - Concurrency/lifecycle/configuration: no new threads, locks, special static lifecycle, or production configuration are introduced. The mixed live-policy/SQL-cache snapshot race remains unresolved in existing discussion 4002131610 and is not reposted.
- Compatibility/persistence/writes/FE-BE variables: there is no storage-format, protocol, symbol, EditLog, transaction/write, rolling-upgrade, or new FE-BE variable change.
- Conditions/error behavior: the inline finding covers both observable failure modes from the same marker-placement root cause; no other conditional or silent-error path was substantiated.
- Tests/results: the ordered regression correctly proves row-policy enforcement on both branches, and the unit test proves movable visible-mask retention, but neither covers a non-movable mask or an exact hidden-control-column mask. Exact-head FE UT evidence showed two failures in unrelated unchanged OSS-HDFS and Iceberg tests. This review was static-only; no build or test was independently run, as required by the review environment.
- Performance/observability/memory/nullability: duplicate provider work belongs to the already-fenced cache-snapshot issue. No separate performance or observability blocker was found; this FE-only change introduces no BE memory or nullable handling.
No additional user review focus was supplied. The review converged after two rounds; both normal Round 2 reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS beyond the one inline comment.
Contributor
TPC-H: Total hot run time: 16648 ms |
Contributor
TPC-DS: Total hot run time: 82140 ms |
Contributor
ClickBench: Total hot run time: 14.74 s |
### What problem does this PR solve?\n\nIssue Number: N/A\n\nRelated PR: apache#67783\n\nProblem Summary: MOW time-travel reconstruction predicates were built above LogicalCheckPolicy. Expanding a non-movable data mask therefore evaluated the mask before commit-TSO, delete-sign, and binlog-operation filters, and masks on hidden control columns could also replace the ExprIds referenced by those parent filters. Wrap each complete reconstruction filter chain in LogicalCheckPolicy and let CheckPolicy absorb every filter in the chain below the mask project. The unit test covers a non-movable assert_true mask and masks the hidden reconstruction columns as well.\n\n### Release note\n\nMOW time-travel reconstruction filters now run before row-policy data masks.\n\n### Check List (For Author)\n\n- Test:\n - Unit Test: CheckRowPolicyTest (11 tests)\n - FE build with Checkstyle\n- Behavior changed: Yes (time-travel-only reconstruction predicates execute below policy masks)\n- Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes for three exact-head correctness failures in MOW time travel.
Critical checkpoint conclusions:
- Goal/data correctness/security: the change enforces ordinary visible-column row policies and preserves masks on both reconstructed branches, but it does not fully accomplish the goal. A valid error-raising row policy can execute on rows outside the historical image; an accepted origin-hidden-column policy cannot bind on the row-binlog branch; and a legal dotted column name is corrupted by the new projection alias. The first two fail closed rather than bypassing RBAC. The still-present mixed-policy/cache-proof leak is already hard-fenced by discussion 4002131610 and is not reposted.
- Scope/clarity/parallel paths/conditions: the six-file change is focused. Both MOW branches, ordinary unique-key filtering, DUP time travel, direct
@incr, views/export, MV refresh, aliases, partitions/tablets, and the widened filter loop were traced. No fourth distinct defect was found. - Concurrency/lifecycle/configuration: no new threads, locks, production configuration, special static initialization, or resource lifecycle are introduced. The only mutable policy interleaving is the existing duplicate-fenced discussion above.
- Compatibility/persistence/writes/FE-BE variables: there is no function-symbol, storage-format, EditLog, transaction/write, FE-BE protocol/variable, or rolling-upgrade change.
- Tests/results: the generated
.outmatches its two labeled ordered queries, and the unit/regression additions cover the ordinary visible-policy and mask-placement happy paths. They do not cover an error-raising row policy, an origin-only hidden policy slot, or a dotted primitive column. The author reports the exact-headCheckRowPolicyTestat 11/11; this review was static-only and did not independently run builds or tests, as required by the review environment. - Error handling/data correctness: the three inline comments identify deterministic analysis/execution failures with concrete plan and value cases. No ignored status, silent fallback, or additional MoW version-boundary defect was found.
- Observability/performance/memory/nullability: no new logging or metrics are needed for this planner-only change. Duplicate provider work shares the already-fenced snapshot issue; no separate performance concern was substantiated. This FE-only diff adds no BE memory or nullable handling.
- Other issues: no additional user review focus was provided. After two full rounds, both normal round-two reviewers and the separate risk reviewer returned
NO_NEW_VALUABLE_FINDINGSbeyond the three inline comments and existing duplicates.
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes at exact head cbb2b9f20230984421a63f51513fa3996f9a11a4.
No new inline comment is added in this pass: the remaining substantiated issues are already covered by existing inline threads, which are hard duplicate fences:
- Use one policy snapshot for both generated branches: both branch markers still read mutable row-filter/data-mask state independently, and the later origin-table entry can overwrite the SQL-cache proof for the earlier branch.
- Map origin policy columns before checking the binlog branch: origin policy ownership is now correct, but an invisible non-key origin column is absent from the row-binlog schema and still cannot bind on the right branch.
- Preserve the reconstruction filter as a row-policy barrier: reconstruction conjuncts and an error-raising/non-movable row policy are still combined into one filter, so the policy can observe rows outside the historical image.
Checkpoint conclusions:
- Goal and evidence: the ordinary visible-column path now applies row filters/data masks to both MOW historical branches, keeps reconstruction filters below mask projects, preserves raw dotted names, and has focused FE plan-shape plus ordered regression coverage. The three unresolved cases above prevent a complete correctness/security conclusion.
- Scope and parallel paths: the production change is focused. I traced both union branches, DUP time travel, direct
@incr, wrapper/MV-refresh identity, set-operation binding, partition/tablet propagation, and qualified/unqualified output binding. No additional unfenced issue was found. - Concurrency: planner nodes add no locks or shared lifecycle state, but authorization state can change between the two independent branch lookups; that is the first blocking existing thread above.
- Lifecycle, configuration, compatibility, and persistence: there is no new production configuration, long-lived owner, edit-log/persistence path, transaction/write path, FE-BE field, wire/storage format, or rolling-upgrade requirement.
- Tests and results: the checked-in
.outmatches the ordered queries and covers a base-branch row, a reconstructed before-image row, and qualified/unqualified dotted columns. The FE structural test covers both mask projects and all three reconstruction-control slots. I did not run builds or tests because this review environment explicitly prohibits them; this conclusion is static-only. The missing race, hidden-column, and error-policy negatives correspond to the existing blocking threads. - Performance and observability: no distinct hot-path regression or new observability requirement was substantiated. The extra policy-source call/latency shares the same cause and fix as the single-snapshot blocking thread.
- Security: restricted authenticated SQL users are in scope under the Doris threat model. The unresolved policy-snapshot path can produce a mixed-policy plan, so this review cannot approve the access-control change yet.
There was no additional user-provided focus. One complete review round (two full reviews plus a separate risk-focused scan) converged with no new valuable findings after duplicate fencing and main-agent verification.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes at exact head 40f0315961e35d8e5b3d180730560ba6439b9f77.
No new inline comment is added in this pass: the remaining substantiated issues are already covered by existing inline threads, which are hard duplicate fences:
- Use one policy snapshot for both generated branches: both branch markers still read mutable row-filter/data-mask state independently, and the later origin-table entry can overwrite the SQL-cache proof for the earlier branch.
- Preserve the reconstruction filter as a row-policy barrier: reconstruction conjuncts and an error-raising/non-movable row policy are still combined into one physical filter, so the policy can observe rows outside the historical image.
- Map origin policy columns before checking the binlog branch: origin policy ownership is correct, but invisible non-key origin columns remain absent from the row-binlog schema and cannot bind on the right branch.
Checkpoint conclusions:
- Goal, correctness, and security: ordinary visible-column row filters and data masks now apply to both MOW historical branches; reconstruction filters are below mask projects; and the latest follow-up correctly preserves raw dotted names plus branch qualifiers. The three unresolved cases above still prevent approval. Restricted authenticated SQL users are in scope under the Doris threat model, and the mixed-policy SQL-cache path can violate the RBAC property by serving rows planned without the retained policy proof.
- Scope, clarity, and parallel paths: the production change is focused. Both MOW branches, DUP time travel, direct
@incr, wrapper/MV-refresh identity, aggregates and filter chains, connector predicates, streams/snapshots, partitions/tablets, union alignment, aliases, and qualified/unqualified binding were traced. No additional unfenced defect was found. - Concurrency and lifecycle: no new threads, locks, static initialization, or resource lifetime are introduced. The mutable policy-provider interleaving between the two sequential branch lookups is the first existing blocker above.
- Configuration, compatibility, persistence, and writes: there is no new production configuration, function symbol, FE-BE field, wire/storage format, EditLog/transaction path, data-write path, or rolling-upgrade requirement.
- Conditions and error handling: the widened filter peel is valid for the reachable wrapper/aggregate shapes, but it does not create an execution barrier between reconstruction and row-policy evaluation; that remains the second existing blocker.
- Tests and results: the generated ordered oracle covers one base-branch row, one reconstructed before-image row, and qualified/unqualified dotted columns; FE tests cover wrapper ownership, mask retention, reconstruction-control slots, and branch qualifiers. Missing cache-mutation, error-policy, and omitted-origin-column negatives correspond to the three existing blockers. This review is static-only: builds and tests were not run because the review environment prohibits them.
- Performance and observability: no distinct hot-path or observability issue was substantiated. The duplicate policy-source call and latency share the same cause and fix as the single-snapshot blocker.
No additional user review focus was provided. One complete review round converged: both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS, and the main-agent final sweep resolved every candidate as an existing duplicate or an evidence-backed dismissal.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 16823 ms |
Contributor
TPC-DS: Total hot run time: 82208 ms |
Contributor
ClickBench: Total hot run time: 14.75 s |
starocean999
approved these changes
Sep 17, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
morningman
added a commit
that referenced
this pull request
Sep 18, 2026
…on PRs from master in merge order (#67783 #68034 #68033 #68057 #68094 #68087) (#68151) Cherry-picked from #67783, #68034, #68033, #68057, #68094, #68087 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each created with `git cherry-pick -x` so the message ends with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830, #68017 and #68073. | # | Master commit | PR | Title | |---|---|---|---| | 1 | e545b13 | #67783 | [fix](policy) Enforce row policies on MOW time travel | | 2 | f7a0842 | #68034 | [fix](binlog) Fix missing DELETE events in row binlog | | 3 | e85575e | #68033 | [fix](binlog) Preserve row binlog compaction policy | | 4 | 21160d7 | #68057 | [fix](binlog) Decouple row binlog compaction from CCR binlog config | | 5 | 04aa5a1 | #68094 | [fix](binlog) Persist row binlog config updates in cloud mode | | 6 | da8feed | #68087 | [fix](binlog) Report streams with missing base tables as stale | Not included on purpose: - The 20 labelled PRs that already carry `incremental-computation-picked` (#62606 in the fork point, #67508 via #67712, the nine of #67830, the six of #68017, the two of #68073, and #68050 whose content this branch got directly through #68012). - #68012 carries the label but is a PR against this branch itself (merged as `6f7c87fa892`); nothing to pick. - #67820 is still open on master; this branch already carries its content via #67861. ### Prerequisite check For every pick I listed the master commits between the fork point (`efedf10c7e3`) and the pick that touch the same files and are not on this branch, and checked whether the pick's behavior depends on them. - **#67783** declares no related PR. It builds on the MOW time-travel rewrite (`BindRelation.buildMowTimeTravelUnion`, #67480) which is already here. Two unlabelled master commits overlap: #66770 (authorization plugin SPI: reworks the data-mask / row-filter API of `LogicalCheckPolicy` and its test) and #67811 (removes the `isPlayNereidsDump()` check in `CheckPolicy`). Neither is a functional prerequisite: the fix consists of `getPolicyTable()` unwrapping `OlapTableWrapper`, `CheckPolicy` collecting the whole filter chain below the policy mask, and `BindRelation` putting a `LogicalCheckPolicy` above each union branch — none of that uses the #66770 API. Not picked; adapted instead (see below). - **#68034**, **#68033**, **#68087** (#68087 relates to #67173, which is before the fork point): no unpicked master commit touches any of their files; the picks applied cleanly and are byte-identical to master. - **#68057**: only `be/test/cloud/cloud_compaction_test.cpp` overlaps with the unrelated #67972 (refresh tablet meta of continuously ingested tablets); auto-merged, the pick only adds new `TEST_F` blocks. Main-code hunks are byte-identical to master. - **#68094**: overlaps with #68090 / #67972 / #66598 (`cloud_tablet.cpp`), #67295 / #67618 / #68090 (`base_tablet.{h,cpp}`), #66598 / #67637 (`meta_service.cpp`), #66598 (`cloud.proto`, `cloud_tablet_test.cpp`) and #67761 / #66598 / #67637 (`meta_service_test.cpp`). All auto-merged. The fix — `BaseTablet::binlog_config()` under the meta lock, `BinlogConfig::operator==`, `CloudTablet::sync_meta()` refreshing the binlog config, `update_tablet` in the meta service accepting `binlog_config`, and `CloudSchemaChangeHandler` routing ROW-binlog property updates through it — does not use anything those commits add. Its hunks are byte-identical to master except one trailing context line in `sync_meta()` (`last_sync_tablet_meta_time_s` comes from #67972). Only #67783 needed adaptation, recorded in its commit message: - `LogicalCheckPolicy.java`: the conflicting context was master's `parsePolicyExpression()` helper (#66770), which does not exist here; the new `getPolicyTable()` is inserted in the same place without it. - `CheckRowPolicyTest.java`: the data-mask mock is written against this branch's per-column `AccessControllerManager.evalDataMaskPolicy(..., column) -> Optional<DataMaskPolicy>` instead of master's `evalDataMaskPolicies(..., Set<String>) -> Map<String, DataMaskSpec>`. The masks are the same (concat for the random-distribution table; the non-movable `k2` mask plus identity masks for the hidden reconstruction columns of the MOW table). The master-only `Or` import (#66770) is not carried; `Collections` / `Locale` imports were added because they arrive with #66770 on master. ### Drift check against master Each pick's `+`/`-` lines are identical to the master commit's, except for the #67783 adaptation above. After the six picks, the touched files still differ from master at `da8feed859d` in: `cloud_tablet.cpp`, `base_tablet.{h,cpp}`, `cloud_compaction_test.cpp`, `cloud_tablet_test.cpp`, `meta_service.cpp`, `meta_service_test.cpp`, `cloud.proto`, `CheckPolicy.java`, `LogicalCheckPolicy.java`, `CheckRowPolicyTest.java`. Replaying the nine unpicked master commits listed above (#67637, #67618, #66770, #66598, #67972, #67811, #67761, #67295, #68090) in a temporary index and removing this branch's own #67861 (`GetTsoRecoveryTransactions` in `cloud.proto` / `meta_service_test.cpp`) brings every file to zero diff against master, except the two `LogicalCheckPolicy.java` / `CheckRowPolicyTest.java` hunks of #66770 that overlap the adaptation. Nothing else is left over. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main + test) with the test classes touched by the picks plus `DeleteFromCommandTest` from #68034's checklist: 5 classes, 58 tests, 0 failures, 0 errors, BUILD SUCCESS — `CheckRowPolicyTest` 9 (the three tests #67783 adds included), `ExplainTableStreamPlanTest` 24, `CloudSchemaChangeHandlerTest` 18 (the tests of #68033 and #68094 included), `DropTableStreamTest` 5, `DeleteFromCommandTest` 2. - FE checkstyle on fe-core: 0 violations. - BE: `-fsyntax-only` with the flags of the Release build (`compile_commands.json`, regenerated `gen_cpp` headers incl. the new `TabletMetaInfoPB.binlog_config`) passes for `cloud/cloud_tablet.cpp`, `cloud/cloud_storage_engine.cpp`, `storage/olap_server.cpp`, `storage/tablet/tablet.cpp`, `storage/tablet/base_tablet.cpp`, and with `-DBE_TEST -fno-access-control` for `test/cloud/cloud_compaction_test.cpp`, `test/cloud/cloud_tablet_test.cpp`, `test/storage/compaction/compaction_task_test.cpp`, `test/storage/tablet/tablet_test.cpp`. - Meta service: the two-line `update_tablet` hunk of #68094 only uses the generated `TabletMetaInfoPB::has_binlog_config()/binlog_config()` and `TabletMetaCloudPB::mutable_binlog_config()` accessors, all present in the regenerated headers. - The two new groovy suites (`row_binlog_p0/test_row_binlog_mow_light_delete`, `time_travel_p0/test_mow_time_travel_row_policy`) parse cleanly (groovy parser check). --------- Co-authored-by: morrySnow <zhangwenxin@selectdb.com> Co-authored-by: Luwei <814383175@qq.com>
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
What problem does this PR solve?
A row policy can be bypassed by a merge-on-write time-travel query. The rewrite replaces the original relation with a subquery alias over a union of the base-table scan and the row-binlog scan. The policy marker left above that composite plan is then removed because policy analysis only recognizes relation-shaped children, so neither union branch receives the row filter.
This can be reproduced by creating a merge-on-write table with historical row binlog enabled, updating and deleting rows after a captured commit TSO, assigning restrictive and permissive row policies to a non-privileged user, and querying the captured version as that user. Rows rejected by the policy are returned from both the base branch and the before-image branch.
What is changed?
OlapTableWrapperlayer to the original table. Policy expressions and data masks are still bound against each wrapper scan's own output slots.Tests
CheckRowPolicyTest: 10 tests passed.test_mow_time_travel_row_policy: passed.test_time_travel_mow: passed.