[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.
fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java
Outdated
Show resolved
Hide resolved
|
|
||
| // 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)) { |
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
3 times, most recently
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
|
Contributor
Author
|
run buildall |
dataroaring
force-pushed
the
mor_value_predicate_pushdown_control
branch
from
b0aa80e to
4fbb3a0
Compare
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 30437 ms |
ClickBench: Total hot run time: 28.26 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
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 |
dataroaring
force-pushed
the
mor_value_predicate_pushdown_control
branch
2 times, most recently
from
0e91aeb to
12559a6
Compare
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 30303 ms |
ClickBench: Total hot run time: 28.25 s |
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 |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
…OR tables without merge Add a per-table session variable that allows reading MOR (Merge-On-Read) UNIQUE tables as DUP tables, skipping storage engine merge and delete sign filter while still honoring delete predicates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dataroaring
force-pushed
the
mor_value_predicate_pushdown_control
branch
from
12559a6 to
dbfbb49
Compare
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 30394 ms |
ClickBench: Total hot run time: 28.25 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 |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
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
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