[feature](scan) Add value predicate pushdown control for MOR tables#60513
Open
dataroaring wants to merge 3 commits intomasterfrom
Open
[feature](scan) Add value predicate pushdown control for MOR tables#60513dataroaring wants to merge 3 commits intomasterfrom
dataroaring wants to merge 3 commits intomasterfrom
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
dataroaring
force-pushed
the
mor_value_predicate_pushdown_control
branch
2 times, most recently
from
fffa13a to
94bc596
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a session variable to control value column predicate pushdown for MOR (Merge-On-Read) tables, allowing users to selectively enable this optimization per table or globally.
Changes:
- Added session variable
enable_mor_value_predicate_pushdown_tablesfor selective table-level control - Added
isMorTable()helper method to identify MOR tables (UNIQUE_KEYS without merge-on-write) - Modified predicate pushdown logic to support value predicates on MOR tables when enabled
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| gensrc/thrift/PlanNodes.thrift | Added optional boolean field enable_mor_value_predicate_pushdown to TOlapScanNode struct |
| fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java | Added session variable declaration, getter, and helper method to check if MOR value predicate pushdown is enabled |
| fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java | Added isMorTable() helper method to identify MOR tables |
| fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java | Set thrift flag in toThrift() based on table type and session variable |
| be/src/pipeline/exec/scan_operator.h | Added virtual method _should_push_down_mor_value_predicate() with default false implementation |
| be/src/pipeline/exec/scan_operator.cpp | Modified predicate pushdown condition to include MOR value predicate check |
| be/src/pipeline/exec/olap_scan_operator.h | Declared override for _should_push_down_mor_value_predicate() |
| be/src/pipeline/exec/olap_scan_operator.cpp | Implemented _should_push_down_mor_value_predicate() to read thrift flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String tblName = olapTable.getName(); | ||
| boolean enabled = ConnectContext.get().getSessionVariable() | ||
| .isMorValuePredicatePushdownEnabled(dbName, tblName); | ||
| msg.olap_scan_node.setEnable_mor_value_predicate_pushdown(enabled); |
There was a problem hiding this comment.
The setter method name appears to be incorrect. Thrift generates Java setter methods with CamelCase naming from snake_case field names. The method should be setEnableMorValuePredicatePushdown (without underscores) instead of setEnable_mor_value_predicate_pushdown. This follows the same pattern as line 1182 where enable_unique_key_merge_on_write becomes setEnableUniqueKeyMergeOnWrite.
Suggested change
| msg.olap_scan_node.setEnable_mor_value_predicate_pushdown(enabled); | |
| msg.olap_scan_node.setEnableMorValuePredicatePushdown(enabled); |
|
|
||
| // Set MOR value predicate pushdown flag based on session variable | ||
| if (olapTable.isMorTable() && ConnectContext.get() != null) { | ||
| String dbName = olapTable.getQualifiedDbName(); |
There was a problem hiding this comment.
Potential null pointer exception. The method getQualifiedDbName() can return null (as seen in Table.java line 367-369). This null value would then be passed to isMorValuePredicatePushdownEnabled() which concatenates it with the table name on line 4691, potentially resulting in "null.tableName". Consider adding a null check for dbName or using an alternative method like getDBName() which handles the null case.
Suggested change
| String dbName = olapTable.getQualifiedDbName(); | |
| String dbName = olapTable.getQualifiedDbName(); | |
| if (dbName == null) { | |
| dbName = olapTable.getDBName(); | |
| } |
| String trimmed = enableMorValuePredicatePushdownTables.trim(); | ||
| if ("*".equals(trimmed)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The method doesn't handle null dbName parameter. If dbName is null, line 4691 will create "null.tableName" which could lead to unexpected behavior. Consider adding a null check for dbName at the beginning of the method, or using Objects.requireNonNull() to fail fast, or handle the null case explicitly by using only tableName for comparison when dbName is null.
Suggested change
| } | |
| } | |
| // When dbName is null, only compare using tableName to avoid creating "null.tableName". | |
| if (dbName == null) { | |
| for (String table : trimmed.split(",")) { | |
| if (table.trim().equalsIgnoreCase(tableName)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
Comment on lines
+4682
to
+4699
| public boolean isMorValuePredicatePushdownEnabled(String dbName, String tableName) { | ||
| if (enableMorValuePredicatePushdownTables == null | ||
| || enableMorValuePredicatePushdownTables.isEmpty()) { | ||
| return false; | ||
| } | ||
| String trimmed = enableMorValuePredicatePushdownTables.trim(); | ||
| if ("*".equals(trimmed)) { | ||
| return true; | ||
| } | ||
| String fullName = dbName + "." + tableName; | ||
| for (String table : trimmed.split(",")) { | ||
| if (table.trim().equalsIgnoreCase(fullName) | ||
| || table.trim().equalsIgnoreCase(tableName)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The new session variable and its helper method isMorValuePredicatePushdownEnabled() lack test coverage. Consider adding unit tests in SessionVariablesTest to verify:
- Empty string handling (default behavior)
- Wildcard '*' behavior
- Single table name matching (with and without database prefix)
- Multiple table names (comma-separated)
- Case-insensitive matching
- Whitespace handling in table names
- Edge cases like null dbName parameter
Comment on lines
+4693
to
+4694
| if (table.trim().equalsIgnoreCase(fullName) | ||
| || table.trim().equalsIgnoreCase(tableName)) { |
There was a problem hiding this comment.
The method doesn't handle edge cases well when splitting by comma. If the variable contains only commas or has consecutive commas (e.g., "db.table1,,db.table2"), the split operation will produce empty strings. Calling trim() on empty strings and comparing them could lead to false positives. Consider filtering out empty strings after trimming, or using a more robust parsing approach.
Suggested change
| if (table.trim().equalsIgnoreCase(fullName) | |
| || table.trim().equalsIgnoreCase(tableName)) { | |
| String normalized = table.trim(); | |
| if (normalized.isEmpty()) { | |
| continue; | |
| } | |
| if (normalized.equalsIgnoreCase(fullName) | |
| || normalized.equalsIgnoreCase(tableName)) { |
Comment on lines
+3028
to
+3030
| */ | ||
| public boolean isMorTable() { | ||
| return getKeysType() == KeysType.UNIQUE_KEYS && !getEnableUniqueKeyMergeOnWrite(); |
There was a problem hiding this comment.
The isMorTable() method definition appears correct, but consider verifying the behavior when tableProperty is null. According to getEnableUniqueKeyMergeOnWrite() (lines 3011-3019), it returns false when tableProperty is null, which means a UNIQUE_KEYS table with null tableProperty would be considered a MOR table. Ensure this is the intended behavior for all edge cases.
Suggested change
| */ | |
| public boolean isMorTable() { | |
| return getKeysType() == KeysType.UNIQUE_KEYS && !getEnableUniqueKeyMergeOnWrite(); | |
| * Only tables with non-null tableProperty are considered for MOR. | |
| */ | |
| public boolean isMorTable() { | |
| return tableProperty != null | |
| && getKeysType() == KeysType.UNIQUE_KEYS | |
| && !getEnableUniqueKeyMergeOnWrite(); |
dataroaring
force-pushed
the
mor_value_predicate_pushdown_control
branch
from
94bc596 to
8eadc41
Compare
Contributor
Author
|
run buildall |
dataroaring
force-pushed
the
mor_value_predicate_pushdown_control
branch
from
8eadc41 to
4a5d300
Compare
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 31991 ms |
ClickBench: Total hot run time: 28.16 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
…own for MOR tables Add a new session variable `enable_mor_value_predicate_pushdown_tables` to allow users to selectively enable value column predicate pushdown for MOR (Merge-On-Read) tables. This can improve query performance by utilizing inverted indexes on value columns for filtering. The session variable accepts: - Comma-separated table names: `db1.tbl1,db2.tbl2` - Wildcard for all MOR tables: `*` - Empty string to disable (default) Changes: - Add session variable in SessionVariable.java with helper method - Add isMorTable() helper in OlapTable.java - Add Thrift field enable_mor_value_predicate_pushdown in TOlapScanNode - Set flag in OlapScanNode.toThrift() based on session variable - Add virtual method _should_push_down_mor_value_predicate() in scan_operator - Implement override in olap_scan_operator to read the flag - Modify predicate pushdown condition in scan_operator.cpp
dataroaring
force-pushed
the
mor_value_predicate_pushdown_control
branch
from
4a5d300 to
a06783e
Compare
dataroaring
changed the title
dataroaring
force-pushed
the
mor_value_predicate_pushdown_control
branch
2 times, most recently
from
9bb1fa0 to
a0385a9
Compare
…bles Enable value column predicates to be pushed down to storage layer for MOR (Merge-On-Read) tables when controlled by the session variable enable_mor_value_predicate_pushdown_tables. This allows inverted indexes on value columns to be utilized for filtering, improving query performance on dedup-only/insert-only MOR workloads. Key changes: - Propagate enable_mor_value_predicate_pushdown flag from thrift scan node through OlapScanner -> ReaderParams -> RowsetReaderContext -> BetaRowsetReader - Extend _should_push_down_value_predicates() to push value predicates for all rowsets when the flag is set - Skip __DORIS_DELETE_SIGN__ predicate during per-segment pushdown to prevent delete markers from being filtered before merge - Revert scan_operator.cpp to only remove VExpr from conjuncts for key columns, preserving VExpr as post-merge safety net for value columns - Add regression tests covering dedup, delete-sign, and delete-predicate scenarios
dataroaring
force-pushed
the
mor_value_predicate_pushdown_control
branch
from
a0385a9 to
7d4aa18
Compare
Contributor
Author
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 31169 ms |
ClickBench: Total hot run time: 28.05 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
The delete sign skip logic in _init_conditions_param() was reading from _reader_context.enable_mor_value_predicate_pushdown, but this flag is only set later in _capture_rs_readers(). Use read_params directly since it's already available as a function parameter. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 31781 ms |
ClickBench: Total hot run time: 28.88 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
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
4 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.
Summary
enable_mor_value_predicate_pushdown_tablesto control value column predicate pushdown for MOR (Merge-On-Read) tablesdb.table,table, or*), thrift flag on TOlapScanNode,isMorTable()helper on OlapTable_should_push_down_value_predicates()for all rowsets; skip__DORIS_DELETE_SIGN__to preserve delete correctness; keep VExpr in conjuncts as post-merge safety netMotivation
MOR tables with inverted indexes on value columns cannot utilize those indexes for filtering because value predicates are not pushed to the storage layer. This feature enables per-segment value predicate pushdown for dedup-only/insert-only MOR workloads where the same key always carries identical values across rowsets, allowing inverted indexes and zone maps to filter data early.
Design
Two-layer predicate approach:
_conjunctsas post-merge safety netDelete sign handling:
__DORIS_DELETE_SIGN__predicate is excluded from per-segment pushdown to prevent delete markers from being filtered before merge, which would cause deleted rows to reappear.Test plan
INSERT INTO (..., __DORIS_DELETE_SIGN__) VALUES (..., 1)DELETE FROM ... WHERE