Copilot commented on code in PR #60513:
URL: https://github.com/apache/doris/pull/60513#discussion_r2765213352
##########
fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java:
##########
@@ -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 tblName = olapTable.getName();
+ boolean enabled = ConnectContext.get().getSessionVariable()
+ .isMorValuePredicatePushdownEnabled(dbName, tblName);
+ msg.olap_scan_node.setEnable_mor_value_predicate_pushdown(enabled);
Review 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`.
```suggestion
msg.olap_scan_node.setEnableMorValuePredicatePushdown(enabled);
```
##########
fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java:
##########
@@ -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();
Review 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.
```suggestion
String dbName = olapTable.getQualifiedDbName();
if (dbName == null) {
dbName = olapTable.getDBName();
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -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;
+ }
Review 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.
```suggestion
}
// 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;
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -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;
+ }
+ String fullName = dbName + "." + tableName;
+ for (String table : trimmed.split(",")) {
+ if (table.trim().equalsIgnoreCase(fullName)
+ || table.trim().equalsIgnoreCase(tableName)) {
+ return true;
+ }
+ }
+ return false;
+ }
Review Comment:
The new session variable and its helper method
`isMorValuePredicatePushdownEnabled()` lack test coverage. Consider adding unit
tests in SessionVariablesTest to verify:
1. Empty string handling (default behavior)
2. Wildcard '*' behavior
3. Single table name matching (with and without database prefix)
4. Multiple table names (comma-separated)
5. Case-insensitive matching
6. Whitespace handling in table names
7. Edge cases like null dbName parameter
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -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;
+ }
+ String fullName = dbName + "." + tableName;
+ for (String table : trimmed.split(",")) {
+ if (table.trim().equalsIgnoreCase(fullName)
+ || table.trim().equalsIgnoreCase(tableName)) {
Review 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.
```suggestion
String normalized = table.trim();
if (normalized.isEmpty()) {
continue;
}
if (normalized.equalsIgnoreCase(fullName)
|| normalized.equalsIgnoreCase(tableName)) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -3022,6 +3022,14 @@ public boolean isUniqKeyMergeOnWrite() {
return getKeysType() == KeysType.UNIQUE_KEYS &&
getEnableUniqueKeyMergeOnWrite();
}
+ /**
+ * Check if this is a MOR (Merge-On-Read) table.
+ * MOR = UNIQUE_KEYS without merge-on-write enabled.
+ */
+ public boolean isMorTable() {
+ return getKeysType() == KeysType.UNIQUE_KEYS &&
!getEnableUniqueKeyMergeOnWrite();
Review 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.
```suggestion
* Only tables with non-null tableProperty are considered for MOR.
*/
public boolean isMorTable() {
return tableProperty != null
&& getKeysType() == KeysType.UNIQUE_KEYS
&& !getEnableUniqueKeyMergeOnWrite();
```
--
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]
