-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[feature](scan) Add value predicate pushdown control for MOR tables #60513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
a06783e
7d4aa18
b445c13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1181,6 +1181,15 @@ protected void toThrift(TPlanNode msg) { | |||||||||||
| msg.olap_scan_node.setTableName(tableName); | ||||||||||||
| msg.olap_scan_node.setEnableUniqueKeyMergeOnWrite(olapTable.getEnableUniqueKeyMergeOnWrite()); | ||||||||||||
|
|
||||||||||||
| // Set MOR value predicate pushdown flag based on session variable | ||||||||||||
| if (olapTable.isMorTable() && ConnectContext.get() != null) { | ||||||||||||
| String dbName = olapTable.getQualifiedDbName(); | ||||||||||||
|
||||||||||||
| String dbName = olapTable.getQualifiedDbName(); | |
| String dbName = olapTable.getQualifiedDbName(); | |
| if (dbName == null) { | |
| dbName = olapTable.getDBName(); | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -722,6 +722,9 @@ public class SessionVariable implements Serializable, Writable { | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| public static final String ENABLE_PUSHDOWN_STRING_MINMAX = "enable_pushdown_string_minmax"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| public static final String ENABLE_MOR_VALUE_PREDICATE_PUSHDOWN_TABLES | ||||||||||||||||||||||||
| = "enable_mor_value_predicate_pushdown_tables"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // When set use fix replica = true, the fixed replica maybe bad, try to use the health one if | ||||||||||||||||||||||||
| // this session variable is set to true. | ||||||||||||||||||||||||
| public static final String FALLBACK_OTHER_REPLICA_WHEN_FIXED_CORRUPT = "fallback_other_replica_when_fixed_corrupt"; | ||||||||||||||||||||||||
|
|
@@ -2181,6 +2184,13 @@ public boolean isEnableHboNonStrictMatchingMode() { | |||||||||||||||||||||||
| "是否启用 string 类型 min max 下推。", "Set whether to enable push down string type minmax."}) | ||||||||||||||||||||||||
| public boolean enablePushDownStringMinMax = false; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Comma-separated list of MOR tables to enable value predicate pushdown. | ||||||||||||||||||||||||
| @VariableMgr.VarAttr(name = ENABLE_MOR_VALUE_PREDICATE_PUSHDOWN_TABLES, needForward = true, description = { | ||||||||||||||||||||||||
| "指定启用MOR表value列谓词下推的表列表,格式:db1.tbl1,db2.tbl2 或 * 表示所有MOR表。", | ||||||||||||||||||||||||
| "Comma-separated list of MOR tables to enable value predicate pushdown. " | ||||||||||||||||||||||||
| + "Format: db1.tbl1,db2.tbl2 or * for all MOR tables."}) | ||||||||||||||||||||||||
| public String enableMorValuePredicatePushdownTables = ""; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Whether drop table when create table as select insert data appear error. | ||||||||||||||||||||||||
| @VariableMgr.VarAttr(name = DROP_TABLE_IF_CTAS_FAILED, needForward = true) | ||||||||||||||||||||||||
| public boolean dropTableIfCtasFailed = true; | ||||||||||||||||||||||||
|
|
@@ -4659,6 +4669,35 @@ public boolean isEnablePushDownStringMinMax() { | |||||||||||||||||||||||
| return enablePushDownStringMinMax; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| public String getEnableMorValuePredicatePushdownTables() { | ||||||||||||||||||||||||
| return enableMorValuePredicatePushdownTables; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * Check if a table is enabled for MOR value predicate pushdown. | ||||||||||||||||||||||||
| * @param dbName database name | ||||||||||||||||||||||||
| * @param tableName table name | ||||||||||||||||||||||||
| * @return true if the table is in the enabled list or if '*' is set | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| public boolean isMorValuePredicatePushdownEnabled(String dbName, String tableName) { | ||||||||||||||||||||||||
| if (enableMorValuePredicatePushdownTables == null | ||||||||||||||||||||||||
| || enableMorValuePredicatePushdownTables.isEmpty()) { | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| String trimmed = enableMorValuePredicatePushdownTables.trim(); | ||||||||||||||||||||||||
| if ("*".equals(trimmed)) { | ||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| } | |
| } | |
| // 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; | |
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| if (table.trim().equalsIgnoreCase(fullName) | |
| || table.trim().equalsIgnoreCase(tableName)) { | |
| String normalized = table.trim(); | |
| if (normalized.isEmpty()) { | |
| continue; | |
| } | |
| if (normalized.equalsIgnoreCase(fullName) | |
| || normalized.equalsIgnoreCase(tableName)) { |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| -- This file is automatically generated. You should know what you did if you want to edit this | ||
| -- !select_disabled -- | ||
| 2 200 world | ||
| 3 300 test | ||
|
|
||
| -- !select_enabled_tablename -- | ||
| 2 200 world | ||
| 3 300 test | ||
|
|
||
| -- !select_enabled_fullname -- | ||
| 2 200 world | ||
| 3 300 test | ||
|
|
||
| -- !select_enabled_wildcard -- | ||
| 2 200 world | ||
| 3 300 test | ||
|
|
||
| -- !select_eq_predicate -- | ||
| 2 200 world | ||
|
|
||
| -- !select_not_in_list -- | ||
| 2 200 world | ||
| 3 300 test | ||
|
|
||
| -- !select_dedup_all -- | ||
| 1 100 first | ||
| 2 300 third | ||
| 3 500 fifth | ||
|
|
||
| -- !select_dedup_eq -- | ||
| 2 300 third | ||
|
|
||
| -- !select_dedup_none -- | ||
|
|
||
| -- !select_delete_range -- | ||
| 3 300 test | ||
|
|
||
| -- !select_delete_eq -- | ||
|
|
||
| -- !select_delete_all -- | ||
| 1 100 hello | ||
| 3 300 test | ||
|
|
||
| -- !select_delpred_range -- | ||
| 3 300 test | ||
|
|
||
| -- !select_delpred_eq -- | ||
|
|
||
| -- !select_delpred_all -- | ||
| 1 100 hello | ||
| 3 300 test | ||
|
|
||
| -- !select_update_disabled_old -- | ||
|
|
||
| -- !select_update_disabled_new -- | ||
| 1 500 new | ||
|
|
||
| -- !select_update_enabled_old -- | ||
| 1 100 old | ||
|
|
||
| -- !select_update_enabled_new -- | ||
| 1 500 new | ||
|
|
||
| -- !select_update_enabled_range -- | ||
| 1 500 new | ||
| 3 300 keep | ||
|
|
||
| -- !select_multiple_tables -- | ||
| 2 200 | ||
|
|
||
| -- !select_mow_table -- | ||
| 2 200 | ||
|
|
||
| -- !select_dup_table -- | ||
| 2 200 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.