github-actions[bot] commented on code in PR #60513: URL: https://github.com/apache/doris/pull/60513#discussion_r2882205776
########## regression-test/suites/data_model_p0/unique/test_mor_value_predicate_pushdown.groovy: ########## @@ -0,0 +1,366 @@ +// 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_mor_value_predicate_pushdown") { + def tbName = "test_mor_value_pred_pushdown" + def dbName = context.config.getDbNameByFile(context.file) + + // Test 1: Basic MOR table with value predicate pushdown (dedup-only scenario) + // This feature is designed for insert-only/dedup-only workloads where + // the same key always has identical values across rowsets. + sql "DROP TABLE IF EXISTS ${tbName}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName} ( + k1 INT, + v1 INT, + v2 VARCHAR(100) + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 3 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true" + ); + """ + + // Insert test data across separate rowsets (dedup-only: same key has same values) + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world'), (3, 300, 'test')" + // Re-insert duplicates to create overlapping rowsets for dedup + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world')" + + // Test: pushdown disabled (default) + sql "SET enable_mor_value_predicate_pushdown_tables = ''" + + // Verify session variable is set correctly + def result = sql "SHOW VARIABLES LIKE 'enable_mor_value_predicate_pushdown_tables'" + assertTrue(result.size() > 0) + assertTrue(result[0][1] == "") + + // Query with value predicate - should return correct results + qt_select_disabled """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Test: enable for specific table (just table name) + sql "SET enable_mor_value_predicate_pushdown_tables = '${tbName}'" + + result = sql "SHOW VARIABLES LIKE 'enable_mor_value_predicate_pushdown_tables'" + assertTrue(result[0][1] == tbName) + + qt_select_enabled_tablename """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Test: enable for specific table with db prefix + sql "SET enable_mor_value_predicate_pushdown_tables = '${dbName}.${tbName}'" + + qt_select_enabled_fullname """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Test: enable for all MOR tables with '*' + sql "SET enable_mor_value_predicate_pushdown_tables = '*'" + + result = sql "SHOW VARIABLES LIKE 'enable_mor_value_predicate_pushdown_tables'" + assertTrue(result[0][1] == "*") + + qt_select_enabled_wildcard """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Test: equality predicate on value column with pushdown + qt_select_eq_predicate """ + SELECT * FROM ${tbName} WHERE v1 = 200 ORDER BY k1 + """ + + // Test: table not in list - pushdown should be disabled + sql "SET enable_mor_value_predicate_pushdown_tables = 'other_table'" + + qt_select_not_in_list """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Test 2: Multiple rowsets with dedup-only pattern + // Verify correctness when same keys appear across many rowsets with identical values + sql "DROP TABLE IF EXISTS ${tbName}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName} ( + k1 INT, + v1 INT, + v2 VARCHAR(100) + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true" + ); + """ + + // Create multiple overlapping rowsets (dedup-only: all versions have same values) + sql "INSERT INTO ${tbName} VALUES (1, 100, 'first'), (2, 300, 'third')" + sql "INSERT INTO ${tbName} VALUES (1, 100, 'first')" + sql "INSERT INTO ${tbName} VALUES (2, 300, 'third'), (3, 500, 'fifth')" + + // Test with pushdown enabled + sql "SET enable_mor_value_predicate_pushdown_tables = '*'" + + // Should return all rows matching predicate after dedup + qt_select_dedup_all """ + SELECT * FROM ${tbName} WHERE v1 >= 100 ORDER BY k1 + """ + + // Equality match on a value that exists + qt_select_dedup_eq """ + SELECT * FROM ${tbName} WHERE v1 = 300 ORDER BY k1 + """ + + // Predicate that matches no rows + qt_select_dedup_none """ + SELECT * FROM ${tbName} WHERE v1 = 999 ORDER BY k1 + """ + + // Test 3: Dedup + delete scenario + // Value columns are identical across rowsets (dedup-only), but some rows are deleted. + // Verifies that __DORIS_DELETE_SIGN__ is NOT pushed per-segment so deletions are honored. + sql "DROP TABLE IF EXISTS ${tbName}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName} ( + k1 INT, + v1 INT, + v2 VARCHAR(100) + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true" + ); + """ + + // Rowset 1: initial data + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world'), (3, 300, 'test')" + // Rowset 2: dedup insert (same key, same values) + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world')" + // Rowset 3: delete k1=2 via insert with __DORIS_DELETE_SIGN__=1 + // Value columns are identical (dedup-only), only delete sign differs + sql "INSERT INTO ${tbName}(k1, v1, v2, __DORIS_DELETE_SIGN__) VALUES (2, 200, 'world', 1)" + + sql "SET enable_mor_value_predicate_pushdown_tables = '*'" + + // Deleted row must not appear even though v1=200 matches the predicate + qt_select_delete_range """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Equality on deleted row's value — should return empty + qt_select_delete_eq """ + SELECT * FROM ${tbName} WHERE v1 = 200 ORDER BY k1 + """ + + // Broader predicate — deleted row still excluded + qt_select_delete_all """ + SELECT * FROM ${tbName} WHERE v1 >= 100 ORDER BY k1 + """ + + // Test 4: Dedup + delete predicate scenario + // DELETE FROM creates a delete predicate stored in rowset metadata. + // Delete predicates go through DeleteHandler, separate from value predicates. + // Verify they work correctly alongside value predicate pushdown. + sql "DROP TABLE IF EXISTS ${tbName}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName} ( + k1 INT, + v1 INT, + v2 VARCHAR(100) + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true" + ); + """ + + // Rowset 1: initial data + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world'), (3, 300, 'test')" + // Rowset 2: dedup insert (same key, same values) + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world')" + // Delete predicate rowset: DELETE FROM creates a predicate, not row markers + sql "DELETE FROM ${tbName} WHERE k1 = 2" + + sql "SET enable_mor_value_predicate_pushdown_tables = '*'" + + // Deleted row must not appear + qt_select_delpred_range """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Equality on deleted row's value — should return empty + qt_select_delpred_eq """ + SELECT * FROM ${tbName} WHERE v1 = 200 ORDER BY k1 + """ + + // Broader predicate — deleted row still excluded + qt_select_delpred_all """ + SELECT * FROM ${tbName} WHERE v1 >= 100 ORDER BY k1 + """ + + // Test 5: Update scenario — proves pushdown is active at storage layer + // k1=1 is updated from v1=100 to v1=500 across two rowsets. + // Comparing results with pushdown disabled vs enabled for the SAME query + // proves per-segment filtering is happening: + // disabled: merge picks latest (v1=500), VExpr filters → empty + // enabled: rowset 2 filtered per-segment (v1=500≠100), old version survives + // merge sees only old version, VExpr passes → stale (1,100,'old') + sql "DROP TABLE IF EXISTS ${tbName}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName} ( + k1 INT, + v1 INT, + v2 VARCHAR(100) + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true" + ); + """ + + // Rowset 1: initial data + sql "INSERT INTO ${tbName} VALUES (1, 100, 'old'), (2, 200, 'keep'), (3, 300, 'keep')" + // Rowset 2: update k1=1 from v1=100 to v1=500 + sql "INSERT INTO ${tbName} VALUES (1, 500, 'new')" + + // --- Pushdown disabled: correct merge-then-filter behavior --- + sql "SET enable_mor_value_predicate_pushdown_tables = ''" + + // v1=100 does not match latest version (v1=500) → empty + qt_select_update_disabled_old """ + SELECT * FROM ${tbName} WHERE v1 = 100 ORDER BY k1 + """ + + // v1=500 matches latest → returns updated row + qt_select_update_disabled_new """ + SELECT * FROM ${tbName} WHERE v1 = 500 ORDER BY k1 + """ + + // --- Pushdown enabled: per-segment filtering observable --- + sql "SET enable_mor_value_predicate_pushdown_tables = '*'" + + // v1=100: rowset 2 (v1=500) filtered per-segment, old version survives. + // Returns stale data — this proves pushdown is filtering at storage layer. + qt_select_update_enabled_old """ Review Comment: **[MEDIUM] Test explicitly asserts stale data as correct behavior**: This test proves that `enable_mor_value_predicate_pushdown` can return **incorrect results** (`1, 100, old` instead of empty) when keys have been updated across rowsets. The `.out` file records this stale result as the expected output. While the test comment explains this is intentional to prove the pushdown is happening, having a test that asserts incorrect query results as "expected" is concerning. Users who enable this session variable may not realize it can return stale data for any key that has been updated. Consider: 1. Adding a prominent WARNING in the session variable description that this feature is **only safe for insert-only/dedup-only workloads** and will produce incorrect results if keys are ever updated. 2. Potentially logging a warning when the session variable is set. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalOlapScan.java: ########## @@ -769,6 +769,13 @@ AGGREGATE KEY (siteid,citycode,username) && getTable().getKeysType() == KeysType.UNIQUE_KEYS) { return; } + // When readMorAsDup is enabled, MOR tables are read as DUP, so uniqueness cannot be guaranteed. + if (getTable().getKeysType() == KeysType.UNIQUE_KEYS + && getTable().isMorTable() + && ConnectContext.get().getSessionVariable().isReadMorAsDupEnabled( + getTable().getQualifiedDbName(), getTable().getName())) { Review Comment: **[HIGH] NPE risk**: `ConnectContext.get()` can return null (e.g., during background tasks, internal metadata operations, or unit tests without thread-local ConnectContext). This will cause a `NullPointerException`. Other places in this PR correctly guard with `ConnectContext.get() != null` (e.g., `SetPreAggStatus.java` line 158, `OlapScanNode.java` line 1194). This location should be consistent: ```java if (ConnectContext.get() != null && getTable().getKeysType() == KeysType.UNIQUE_KEYS && getTable().isMorTable() && ConnectContext.get().getSessionVariable().isReadMorAsDupEnabled( getTable().getQualifiedDbName(), getTable().getName())) { return; } ``` Note: the pre-existing `skipDeleteBitmap` check at line 768 has the same NPE risk. ########## be/src/olap/rowset/beta_rowset_reader.cpp: ########## @@ -361,7 +361,8 @@ bool BetaRowsetReader::_should_push_down_value_predicates() const { (((_rowset->start_version() == 0 || _rowset->start_version() == 2) && !_rowset->_rowset_meta->is_segments_overlapping() && _read_context->sequence_id_idx == -1) || - _read_context->enable_unique_key_merge_on_write); + _read_context->enable_unique_key_merge_on_write || + _read_context->enable_mor_value_predicate_pushdown); } Review Comment: **[HIGH] Sequence column safety bypass**: The new `enable_mor_value_predicate_pushdown` flag is OR'd unconditionally, bypassing the existing safety check `_read_context->sequence_id_idx == -1`. When a MOR table has a sequence column, the merge winner is determined by the sequence value (not version order). Pushing value predicates to individual segments can filter out the correct merge winner, causing a different (wrong) row to survive merge. **Example**: key=1 in Segment A has `(value=10, seq=100)` and in Segment B has `(value=20, seq=50)`. With predicate `value > 15`: - Without pushdown: merge picks A (seq=100 wins), then predicate filters it → empty. **Correct.** - With pushdown: A filtered at segment level, B passes. Merge sees only B → returns `(key=1, value=20)`. **Wrong** — A should have won. Consider either: 1. Adding a `sequence_id_idx == -1` guard to this branch as well, or 2. Documenting this limitation clearly in the session variable description and adding a WARNING log when the flag is enabled on a table with a sequence column. ########## be/src/vec/exec/scan/olap_scanner.cpp: ########## @@ -478,6 +481,13 @@ Status OlapScanner::_init_tablet_reader_params( if (!_state->skip_storage_engine_merge()) { auto* olap_scan_local_state = (pipeline::OlapScanLocalState*)_local_state; TOlapScanNode& olap_scan_node = olap_scan_local_state->olap_scan_node(); + + // Set MOR value predicate pushdown flag + if (olap_scan_node.__isset.enable_mor_value_predicate_pushdown && + olap_scan_node.enable_mor_value_predicate_pushdown) { + _tablet_reader_params.enable_mor_value_predicate_pushdown = true; + } Review Comment: **[MEDIUM] `enable_mor_value_predicate_pushdown` is only set when `!skip_storage_engine_merge()`**: This means when `read_mor_as_dup` is active (which sets `direct_mode = true` and skips merge at line 340), this flag is NOT set. This is correct because in DUP mode there's no merge to interfere with. But the interaction between these two flags should be documented — currently a reader has to trace through both code paths to understand this. Also note: at line 337, `olap_local_state` is obtained via proper `static_cast`, but at line 480 (just above this block), `olap_scan_local_state` is obtained via C-style cast `(pipeline::OlapScanLocalState*)`. Please use consistent `static_cast` and consider reusing the variable from line 337 to avoid the redundant cast. ########## regression-test/suites/data_model_p0/unique/test_mor_value_predicate_pushdown.groovy: ########## @@ -0,0 +1,366 @@ +// 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_mor_value_predicate_pushdown") { + def tbName = "test_mor_value_pred_pushdown" + def dbName = context.config.getDbNameByFile(context.file) + + // Test 1: Basic MOR table with value predicate pushdown (dedup-only scenario) + // This feature is designed for insert-only/dedup-only workloads where + // the same key always has identical values across rowsets. + sql "DROP TABLE IF EXISTS ${tbName}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName} ( + k1 INT, + v1 INT, + v2 VARCHAR(100) + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 3 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true" + ); + """ + + // Insert test data across separate rowsets (dedup-only: same key has same values) + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world'), (3, 300, 'test')" + // Re-insert duplicates to create overlapping rowsets for dedup + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world')" + + // Test: pushdown disabled (default) + sql "SET enable_mor_value_predicate_pushdown_tables = ''" + + // Verify session variable is set correctly + def result = sql "SHOW VARIABLES LIKE 'enable_mor_value_predicate_pushdown_tables'" + assertTrue(result.size() > 0) + assertTrue(result[0][1] == "") + + // Query with value predicate - should return correct results + qt_select_disabled """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Test: enable for specific table (just table name) + sql "SET enable_mor_value_predicate_pushdown_tables = '${tbName}'" + + result = sql "SHOW VARIABLES LIKE 'enable_mor_value_predicate_pushdown_tables'" + assertTrue(result[0][1] == tbName) + + qt_select_enabled_tablename """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Test: enable for specific table with db prefix + sql "SET enable_mor_value_predicate_pushdown_tables = '${dbName}.${tbName}'" + + qt_select_enabled_fullname """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Test: enable for all MOR tables with '*' + sql "SET enable_mor_value_predicate_pushdown_tables = '*'" + + result = sql "SHOW VARIABLES LIKE 'enable_mor_value_predicate_pushdown_tables'" + assertTrue(result[0][1] == "*") + + qt_select_enabled_wildcard """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Test: equality predicate on value column with pushdown + qt_select_eq_predicate """ + SELECT * FROM ${tbName} WHERE v1 = 200 ORDER BY k1 + """ + + // Test: table not in list - pushdown should be disabled + sql "SET enable_mor_value_predicate_pushdown_tables = 'other_table'" + + qt_select_not_in_list """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Test 2: Multiple rowsets with dedup-only pattern + // Verify correctness when same keys appear across many rowsets with identical values + sql "DROP TABLE IF EXISTS ${tbName}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName} ( + k1 INT, + v1 INT, + v2 VARCHAR(100) + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true" + ); + """ + + // Create multiple overlapping rowsets (dedup-only: all versions have same values) + sql "INSERT INTO ${tbName} VALUES (1, 100, 'first'), (2, 300, 'third')" + sql "INSERT INTO ${tbName} VALUES (1, 100, 'first')" + sql "INSERT INTO ${tbName} VALUES (2, 300, 'third'), (3, 500, 'fifth')" + + // Test with pushdown enabled + sql "SET enable_mor_value_predicate_pushdown_tables = '*'" + + // Should return all rows matching predicate after dedup + qt_select_dedup_all """ + SELECT * FROM ${tbName} WHERE v1 >= 100 ORDER BY k1 + """ + + // Equality match on a value that exists + qt_select_dedup_eq """ + SELECT * FROM ${tbName} WHERE v1 = 300 ORDER BY k1 + """ + + // Predicate that matches no rows + qt_select_dedup_none """ + SELECT * FROM ${tbName} WHERE v1 = 999 ORDER BY k1 + """ + + // Test 3: Dedup + delete scenario + // Value columns are identical across rowsets (dedup-only), but some rows are deleted. + // Verifies that __DORIS_DELETE_SIGN__ is NOT pushed per-segment so deletions are honored. + sql "DROP TABLE IF EXISTS ${tbName}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName} ( + k1 INT, + v1 INT, + v2 VARCHAR(100) + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true" + ); + """ + + // Rowset 1: initial data + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world'), (3, 300, 'test')" + // Rowset 2: dedup insert (same key, same values) + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world')" + // Rowset 3: delete k1=2 via insert with __DORIS_DELETE_SIGN__=1 + // Value columns are identical (dedup-only), only delete sign differs + sql "INSERT INTO ${tbName}(k1, v1, v2, __DORIS_DELETE_SIGN__) VALUES (2, 200, 'world', 1)" + + sql "SET enable_mor_value_predicate_pushdown_tables = '*'" + + // Deleted row must not appear even though v1=200 matches the predicate + qt_select_delete_range """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Equality on deleted row's value — should return empty + qt_select_delete_eq """ + SELECT * FROM ${tbName} WHERE v1 = 200 ORDER BY k1 + """ + + // Broader predicate — deleted row still excluded + qt_select_delete_all """ + SELECT * FROM ${tbName} WHERE v1 >= 100 ORDER BY k1 + """ + + // Test 4: Dedup + delete predicate scenario + // DELETE FROM creates a delete predicate stored in rowset metadata. + // Delete predicates go through DeleteHandler, separate from value predicates. + // Verify they work correctly alongside value predicate pushdown. + sql "DROP TABLE IF EXISTS ${tbName}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName} ( + k1 INT, + v1 INT, + v2 VARCHAR(100) + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true" + ); + """ + + // Rowset 1: initial data + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world'), (3, 300, 'test')" + // Rowset 2: dedup insert (same key, same values) + sql "INSERT INTO ${tbName} VALUES (1, 100, 'hello'), (2, 200, 'world')" + // Delete predicate rowset: DELETE FROM creates a predicate, not row markers + sql "DELETE FROM ${tbName} WHERE k1 = 2" + + sql "SET enable_mor_value_predicate_pushdown_tables = '*'" + + // Deleted row must not appear + qt_select_delpred_range """ + SELECT * FROM ${tbName} WHERE v1 > 150 ORDER BY k1 + """ + + // Equality on deleted row's value — should return empty + qt_select_delpred_eq """ + SELECT * FROM ${tbName} WHERE v1 = 200 ORDER BY k1 + """ + + // Broader predicate — deleted row still excluded + qt_select_delpred_all """ + SELECT * FROM ${tbName} WHERE v1 >= 100 ORDER BY k1 + """ + + // Test 5: Update scenario — proves pushdown is active at storage layer + // k1=1 is updated from v1=100 to v1=500 across two rowsets. + // Comparing results with pushdown disabled vs enabled for the SAME query + // proves per-segment filtering is happening: + // disabled: merge picks latest (v1=500), VExpr filters → empty + // enabled: rowset 2 filtered per-segment (v1=500≠100), old version survives + // merge sees only old version, VExpr passes → stale (1,100,'old') + sql "DROP TABLE IF EXISTS ${tbName}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName} ( + k1 INT, + v1 INT, + v2 VARCHAR(100) + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true" + ); + """ + + // Rowset 1: initial data + sql "INSERT INTO ${tbName} VALUES (1, 100, 'old'), (2, 200, 'keep'), (3, 300, 'keep')" + // Rowset 2: update k1=1 from v1=100 to v1=500 + sql "INSERT INTO ${tbName} VALUES (1, 500, 'new')" + + // --- Pushdown disabled: correct merge-then-filter behavior --- + sql "SET enable_mor_value_predicate_pushdown_tables = ''" + + // v1=100 does not match latest version (v1=500) → empty + qt_select_update_disabled_old """ + SELECT * FROM ${tbName} WHERE v1 = 100 ORDER BY k1 + """ + + // v1=500 matches latest → returns updated row + qt_select_update_disabled_new """ + SELECT * FROM ${tbName} WHERE v1 = 500 ORDER BY k1 + """ + + // --- Pushdown enabled: per-segment filtering observable --- + sql "SET enable_mor_value_predicate_pushdown_tables = '*'" + + // v1=100: rowset 2 (v1=500) filtered per-segment, old version survives. + // Returns stale data — this proves pushdown is filtering at storage layer. + qt_select_update_enabled_old """ + SELECT * FROM ${tbName} WHERE v1 = 100 ORDER BY k1 + """ + + // v1=500: rowset 1 (v1=100) filtered per-segment, new version passes → correct + qt_select_update_enabled_new """ + SELECT * FROM ${tbName} WHERE v1 = 500 ORDER BY k1 + """ + + // v1 > 200: old v1=100 filtered, new v1=500 passes, k1=3 v1=300 passes → correct + qt_select_update_enabled_range """ + SELECT * FROM ${tbName} WHERE v1 > 200 ORDER BY k1 + """ + + // Test 6: Multiple tables in the list + def tbName2 = "test_mor_value_pred_pushdown_2" + sql "DROP TABLE IF EXISTS ${tbName2}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbName2} ( + k1 INT, + v1 INT + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "false" + ); + """ + + sql "INSERT INTO ${tbName2} VALUES (1, 100), (2, 200)" + + sql "SET enable_mor_value_predicate_pushdown_tables = '${tbName},${tbName2}'" + + qt_select_multiple_tables """ + SELECT * FROM ${tbName2} WHERE v1 > 100 ORDER BY k1 + """ + + // Test 7: Non-MOR table (MOW) - value predicates should always be pushed down + // The session variable should have no effect on MOW tables + def tbNameMow = "test_mow_value_pred" + sql "DROP TABLE IF EXISTS ${tbNameMow}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbNameMow} ( + k1 INT, + v1 INT + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "true" + ); + """ + + sql "INSERT INTO ${tbNameMow} VALUES (1, 100), (2, 200)" + + // MOW tables always push down value predicates regardless of setting + sql "SET enable_mor_value_predicate_pushdown_tables = ''" + + qt_select_mow_table """ + SELECT * FROM ${tbNameMow} WHERE v1 > 100 ORDER BY k1 + """ + + // Test 8: DUP_KEYS table - value predicates should always be pushed down + // The session variable should have no effect on DUP_KEYS tables + def tbNameDup = "test_dup_value_pred" + sql "DROP TABLE IF EXISTS ${tbNameDup}" + sql """ + CREATE TABLE IF NOT EXISTS ${tbNameDup} ( + k1 INT, + v1 INT + ) + DUPLICATE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1" + ); + """ + + sql "INSERT INTO ${tbNameDup} VALUES (1, 100), (2, 200)" + + // DUP_KEYS tables always push down value predicates regardless of setting + qt_select_dup_table """ + SELECT * FROM ${tbNameDup} WHERE v1 > 100 ORDER BY k1 + """ + + // Cleanup + sql "SET enable_mor_value_predicate_pushdown_tables = ''" Review Comment: **[LOW] Test style**: Per coding standards, tables should be dropped *before* use (not after tests end), to preserve the environment for debugging. This cleanup block at the end should be removed — the `DROP TABLE IF EXISTS` before each `CREATE TABLE` already handles cleanup. ########## regression-test/suites/unique_with_mor_p0/test_read_mor_as_dup.groovy: ########## @@ -0,0 +1,119 @@ +// 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_read_mor_as_dup") { + def tableName = "test_read_mor_as_dup_tbl" + def tableName2 = "test_read_mor_as_dup_tbl2" + + sql "DROP TABLE IF EXISTS ${tableName}" + sql "DROP TABLE IF EXISTS ${tableName2}" + + // Create a MOR (Merge-On-Read) UNIQUE table + sql """ + CREATE TABLE ${tableName} ( + `k` int NOT NULL, + `v1` int NULL, + `v2` varchar(100) NULL + ) ENGINE=OLAP + UNIQUE KEY(`k`) + DISTRIBUTED BY HASH(`k`) BUCKETS 1 + PROPERTIES ( + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true", + "replication_num" = "1" + ); + """ + + // Create a second MOR table for per-table control test + sql """ + CREATE TABLE ${tableName2} ( + `k` int NOT NULL, + `v1` int NULL + ) ENGINE=OLAP + UNIQUE KEY(`k`) + DISTRIBUTED BY HASH(`k`) BUCKETS 1 + PROPERTIES ( + "enable_unique_key_merge_on_write" = "false", + "disable_auto_compaction" = "true", + "replication_num" = "1" + ); + """ + + // Insert multiple versions of the same key + sql "INSERT INTO ${tableName} VALUES (1, 10, 'first');" + sql "INSERT INTO ${tableName} VALUES (1, 20, 'second');" + sql "INSERT INTO ${tableName} VALUES (1, 30, 'third');" + sql "INSERT INTO ${tableName} VALUES (2, 100, 'only_version');" + + // Insert a row and then delete it + sql "INSERT INTO ${tableName} VALUES (3, 50, 'to_be_deleted');" + sql "DELETE FROM ${tableName} WHERE k = 3;" + + // Insert into second table + sql "INSERT INTO ${tableName2} VALUES (1, 10);" + sql "INSERT INTO ${tableName2} VALUES (1, 20);" + + // === Test 1: Normal query returns merged result === + sql "SET read_mor_as_dup_tables = '';" + def normalResult = sql "SELECT * FROM ${tableName} ORDER BY k;" + // Should see only latest version per key: (1,30,'third'), (2,100,'only_version') + // Key 3 was deleted, should not appear + assertTrue(normalResult.size() == 2, "Normal query should return 2 rows (merged), got ${normalResult.size()}") + + // === Test 2: Wildcard — read all MOR tables as DUP === + sql "SET read_mor_as_dup_tables = '*';" + def dupResult = sql "SELECT * FROM ${tableName} ORDER BY k, v1;" + // Should see all row versions for key 1 (3 versions) + key 2 (1 version) + // Key 3: delete predicates are still applied, so deleted row should still be filtered + // But the delete sign filter is skipped, so we may see it depending on how delete was done. + // For MOR tables, DELETE adds a delete predicate in rowsets, which IS still honored. + assertTrue(dupResult.size() >= 4, "DUP mode should return at least 4 rows (all versions), got ${dupResult.size()}") + + // Verify key 1 has multiple versions + def key1Rows = dupResult.findAll { it[0] == 1 } + assertTrue(key1Rows.size() == 3, "Key 1 should have 3 versions in DUP mode, got ${key1Rows.size()}") + + // === Test 3: Per-table control with db.table format === + def dbName = sql "SELECT DATABASE();" + def currentDb = dbName[0][0] + + sql "SET read_mor_as_dup_tables = '${currentDb}.${tableName}';" + + // tableName should be in DUP mode + def perTableResult = sql "SELECT * FROM ${tableName} ORDER BY k, v1;" + assertTrue(perTableResult.size() >= 4, "Per-table DUP mode should return all versions, got ${perTableResult.size()}") + + // tableName2 should still be in normal (merged) mode + def table2Result = sql "SELECT * FROM ${tableName2} ORDER BY k;" + assertTrue(table2Result.size() == 1, "Table2 should still be merged (1 row), got ${table2Result.size()}") + + // === Test 4: Per-table control with just table name === + sql "SET read_mor_as_dup_tables = '${tableName2}';" + + // tableName should now be in normal mode + def revertedResult = sql "SELECT * FROM ${tableName} ORDER BY k;" + assertTrue(revertedResult.size() == 2, "Table1 should be merged again (2 rows), got ${revertedResult.size()}") + + // tableName2 should be in DUP mode + def table2DupResult = sql "SELECT * FROM ${tableName2} ORDER BY k, v1;" + assertTrue(table2DupResult.size() == 2, "Table2 in DUP mode should return 2 rows, got ${table2DupResult.size()}") + + // === Cleanup === + sql "SET read_mor_as_dup_tables = '';" Review Comment: **[LOW] Test style**: Same issue as the other test — tables should not be dropped at the end of tests. Remove this cleanup block to preserve tables for debugging. -- 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]
