github-actions[bot] commented on code in PR #67783:
URL: https://github.com/apache/doris/pull/67783#discussion_r4002131602
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -601,7 +603,8 @@ private LogicalPlan buildMowTimeTravelUnion(LogicalOlapScan
baseScan, OlapTable
.collect(Collectors.toList());
// left: base survived rows at t1 = delete_sign=0 AND commit_tso <
targetTso, projected to visible.
- LogicalPlan left = checkAndAddDeleteSignFilter(baseScan,
ConnectContext.get(), olapTable, true);
+ LogicalPlan left = checkAndAddDeleteSignFilter(
+ baseScan, ConnectContext.get(), olapTable, true, true);
Review Comment:
[P1] Preserve masked outputs in the branch projection
For a mask on visible slot `v`, policy expansion produces:
```text
Project(v#raw AS v#branch)
Filter(delete/tso)
Project(mask(v#raw) AS v#masked, hidden...)
Scan(v#raw, hidden...)
```
The hidden-slot filter can push below the mask project. Project merging then
has a replacement keyed by `v#masked`, not the parent's `v#raw`, and replaces
the masking project, leaving `Project(v#raw AS v#branch) -> Filter -> Scan`;
the right branch has the same shape. Thus ordinary movable masks expose raw
historical values, while non-movable variants can fail child-output validation.
Please make each branch projection consume post-policy slots and add a MOW
time-travel mask test for both branches.
##########
regression-test/suites/time_travel_p0/test_mow_time_travel_row_policy.groovy:
##########
@@ -0,0 +1,104 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_mow_time_travel_row_policy", "nonConcurrent,p0,auth") {
+ if (isCloudMode()) {
+ return
+ }
+
+ String dbName = "test_mow_time_travel_row_policy_db"
+ String tableName = "test_mow_time_travel_row_policy_table"
+ String user = "test_mow_time_travel_row_policy_user"
+ String password = "C123_567p"
+ String restrictivePolicy = "test_mow_time_travel_value_policy"
+ String firstKeyPolicy = "test_mow_time_travel_first_key_policy"
+ String thirdKeyPolicy = "test_mow_time_travel_third_key_policy"
+
+ try_sql "DROP ROW POLICY IF EXISTS ${restrictivePolicy} ON
${dbName}.${tableName} FOR ${user}"
+ try_sql "DROP ROW POLICY IF EXISTS ${firstKeyPolicy} ON
${dbName}.${tableName} FOR ${user}"
+ try_sql "DROP ROW POLICY IF EXISTS ${thirdKeyPolicy} ON
${dbName}.${tableName} FOR ${user}"
+ try_sql "DROP USER IF EXISTS ${user}"
+ sql "DROP DATABASE IF EXISTS ${dbName}"
+
+ try {
+ sql "CREATE DATABASE ${dbName}"
+ sql """
+ CREATE TABLE ${dbName}.${tableName} (
+ k INT,
+ v INT
+ )
+ UNIQUE KEY(k)
+ DISTRIBUTED BY HASH(k) BUCKETS 1
+ PROPERTIES (
+ 'replication_num' = '1',
+ 'enable_unique_key_merge_on_write' = 'true',
+ 'binlog.enable' = 'true',
+ 'binlog.format' = 'ROW',
+ 'binlog.need_historical_value' = 'true'
+ )
+ """
+
+ sql "INSERT INTO ${dbName}.${tableName} VALUES (1, 10), (2, 20), (3,
30), (5, 50)"
+ sql "SET show_hidden_columns = true"
+ long snapshotTso = sql("SELECT MAX(__DORIS_COMMIT_TSO_COL__) FROM
${dbName}.${tableName}")[0][0] as Long
+ sql "SET show_hidden_columns = false"
+
+ // The historical left branch contains unchanged keys 1 and 5. The
row-binlog right branch
+ // restores updated key 2 and deleted key 3. A new key 4 must not
appear in the snapshot.
+ sql "INSERT INTO ${dbName}.${tableName} VALUES (2, 200)"
+ sql "DELETE FROM ${dbName}.${tableName} WHERE k = 3"
+ sql "INSERT INTO ${dbName}.${tableName} VALUES (4, 40)"
+
+ sql "CREATE USER '${user}' IDENTIFIED BY '${password}'"
+ sql "GRANT SELECT_PRIV ON internal.${dbName}.${tableName} TO ${user}"
+ sql """
+ CREATE ROW POLICY ${restrictivePolicy} ON ${dbName}.${tableName}
+ AS RESTRICTIVE TO ${user} USING (v <= 30)
+ """
+ sql """
+ CREATE ROW POLICY ${firstKeyPolicy} ON ${dbName}.${tableName}
+ AS PERMISSIVE TO ${user} USING (k = 1)
+ """
+ sql """
+ CREATE ROW POLICY ${thirdKeyPolicy} ON ${dbName}.${tableName}
+ AS PERMISSIVE TO ${user} USING (k = 3)
+ """
+
+ String userJdbcUrl = org.apache.doris.regression.Config.buildUrlWithDb(
+ context.config.jdbcUrl, dbName)
+ connect(user, password, userJdbcUrl) {
+ sql "SET enable_nereids_planner = true"
+ sql "SET enable_fallback_to_original_planner = false"
+ sql "SET enable_sql_cache = false"
+
+ // Restrictive AND (permissive OR permissive) leaves only key 1 in
the latest image.
+ assertEquals([[1, 10]], sql("SELECT k, v FROM ${tableName} ORDER
BY k"))
Review Comment:
[P2] Follow the regression oracle and cleanup contract
These ordered, deterministic results should be recorded through labeled
`order_qt` queries with the script-generated `.out`, rather than hand-authored
`assertEquals` values. The `finally` block also removes the table/database
state even on failure, despite the repository rule to clean up before setup and
preserve post-test state for debugging. Please convert these checks to
generated query-test oracles and retain the created table state after the run.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -624,7 +627,7 @@ private LogicalPlan buildMowTimeTravelUnion(LogicalOlapScan
baseScan, OlapTable
binlogScan = binlogScan.withTableScanParams(
new TableScanParams(TableScanParams.INCREMENTAL_READ,
incrParams, Lists.newArrayList()));
- LogicalPlan right = checkAndAddChangeScanFilter(binlogScan,
StreamScanType.MIN_DELTA, true);
+ LogicalPlan right = checkAndAddChangeScanFilter(binlogScan,
StreamScanType.MIN_DELTA, true, true);
Review Comment:
[P1] Use one policy snapshot for both generated branches
These two markers independently read live row-filter/data-mask state, but
`SqlCacheContext` retains only one value per original table/column, so the
second branch overwrites the first branch's proof. For example, if the left
branch sees no row filter and policy `P` is created before the right branch
sees `P`, the result still contains unfiltered left rows while the cache
records only `P`; a later cache check sees current `P == P` and can serve those
forbidden rows. Resolve the policy once and reuse it for both branches, or
otherwise fail safe/disable caching for a mixed snapshot, with a deterministic
policy-change test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
