Copilot commented on code in PR #61034:
URL: https://github.com/apache/doris/pull/61034#discussion_r2882222270
##########
regression-test/suites/insert_p0/insert_group_commit_into.groovy:
##########
@@ -479,6 +479,39 @@ suite("insert_group_commit_into") {
exception """null value for not null column"""
}
getRowCount(2)
+
+ // test invalid group_commit_interval_ms and group_commit_data_bytes
+ test {
+ sql """ ALTER TABLE ${table} SET ("group_commit_interval_ms" =
"0"); """
+ exception """group_commit_interval_ms must be greater than 0"""
+ }
+ test {
+ sql """ ALTER TABLE ${table} SET ("group_commit_data_bytes" =
"0"); """
+ exception """group_commit_data_bytes must be greater than 0"""
+ }
+ test {
+ sql """ CREATE TABLE IF NOT EXISTS ${table}_invalid ( k1 INT not
null )
+ DUPLICATE KEY(`k1`)
+ DISTRIBUTED BY HASH(`k1`)
+ BUCKETS 1 PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "group_commit_interval_ms" = "0",
+ "group_commit_data_bytes" = "100"
+ );
+ """
+ exception """group_commit_interval_ms must be greater than 0"""
+ }
+ test {
+ sql """ CREATE TABLE IF NOT EXISTS ${table}_invalid ( k1 INT not
null )
+ DUPLICATE KEY(`k1`)
+ DISTRIBUTED BY HASH(`k1`)
+ BUCKETS 1 PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "group_commit_data_bytes" = "0"
+ );
+ """
Review Comment:
This second CREATE TABLE test reuses the same table name `${table}_invalid`
as the previous one and still uses `IF NOT EXISTS`. Reusing the name can make
the test order-dependent (e.g., if the first CREATE unexpectedly succeeds, this
one may be skipped). Use a different table name per case or drop the table
between tests.
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java:
##########
@@ -1869,24 +1869,35 @@ public static int
analyzeGroupCommitIntervalMs(Map<String, String> properties) t
} catch (Exception e) {
throw new AnalysisException("parse group_commit_interval_ms
format error");
}
+ if (groupCommitIntervalMs <= 0) {
+ throw new AnalysisException("group_commit_interval_ms must be
greater than 0");
+ }
Review Comment:
The PR title suggests fixing an inability to set these properties to 0, but
the new validation explicitly rejects 0 (and tests assert it fails). Please
clarify the intended behavior and align the PR title/description (and tests)
accordingly; if 0 is meant to be allowed (e.g., to disable a threshold), this
check should not reject it.
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java:
##########
@@ -1869,24 +1869,35 @@ public static int
analyzeGroupCommitIntervalMs(Map<String, String> properties) t
} catch (Exception e) {
throw new AnalysisException("parse group_commit_interval_ms
format error");
}
Review Comment:
The parse-error AnalysisExceptions here are very generic (e.g., they don’t
include the invalid value), and this method is now used by ALTER TABLE
validation too. Consider including the offending value in the error message to
make debugging easier (similar to the previous messages in
ModifyTablePropertiesOp).
##########
regression-test/suites/insert_p0/insert_group_commit_into.groovy:
##########
@@ -479,6 +479,39 @@ suite("insert_group_commit_into") {
exception """null value for not null column"""
}
getRowCount(2)
+
+ // test invalid group_commit_interval_ms and group_commit_data_bytes
+ test {
+ sql """ ALTER TABLE ${table} SET ("group_commit_interval_ms" =
"0"); """
+ exception """group_commit_interval_ms must be greater than 0"""
+ }
+ test {
+ sql """ ALTER TABLE ${table} SET ("group_commit_data_bytes" =
"0"); """
+ exception """group_commit_data_bytes must be greater than 0"""
+ }
+ test {
+ sql """ CREATE TABLE IF NOT EXISTS ${table}_invalid ( k1 INT not
null )
+ DUPLICATE KEY(`k1`)
+ DISTRIBUTED BY HASH(`k1`)
+ BUCKETS 1 PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "group_commit_interval_ms" = "0",
+ "group_commit_data_bytes" = "100"
+ );
+ """
+ exception """group_commit_interval_ms must be greater than 0"""
+ }
+ test {
+ sql """ CREATE TABLE IF NOT EXISTS ${table}_invalid ( k1 INT not
null )
+ DUPLICATE KEY(`k1`)
+ DISTRIBUTED BY HASH(`k1`)
+ BUCKETS 1 PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "group_commit_data_bytes" = "0"
+ );
+ """
Review Comment:
This CREATE TABLE negative test uses `IF NOT EXISTS` even though it expects
an error. If `${table}_invalid` already exists from a prior run or partial
creation, the statement may no-op and the test won't validate the error path.
Consider removing `IF NOT EXISTS` (or explicitly dropping the table first).
```suggestion
);
"""
exception """group_commit_interval_ms must be greater than 0"""
}
sql """ drop table if exists ${table}_invalid; """
test {
sql """ CREATE TABLE ${table}_invalid ( k1 INT not null )
DUPLICATE KEY(`k1`)
DISTRIBUTED BY HASH(`k1`)
BUCKETS 1 PROPERTIES (
"replication_allocation" =
"tag.location.default: 1",
"group_commit_interval_ms" = "0",
"group_commit_data_bytes" = "100"
);
"""
exception """group_commit_interval_ms must be greater than 0"""
}
sql """ drop table if exists ${table}_invalid; """
test {
sql """ CREATE TABLE ${table}_invalid ( k1 INT not null )
DUPLICATE KEY(`k1`)
DISTRIBUTED BY HASH(`k1`)
BUCKETS 1 PROPERTIES (
"replication_allocation" =
"tag.location.default: 1",
"group_commit_data_bytes" = "0"
);
"""
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyTablePropertiesOp.java:
##########
@@ -310,33 +310,11 @@ public void validate(ConnectContext ctx) throws
UserException {
this.needTableStable = false;
this.opType = AlterOpType.MODIFY_TABLE_PROPERTY_SYNC;
} else if
(properties.containsKey(PropertyAnalyzer.PROPERTIES_GROUP_COMMIT_INTERVAL_MS)) {
- long groupCommitIntervalMs;
- String groupCommitIntervalMsStr =
properties.get(PropertyAnalyzer.PROPERTIES_GROUP_COMMIT_INTERVAL_MS);
- try {
- groupCommitIntervalMs =
Long.parseLong(groupCommitIntervalMsStr);
- if (groupCommitIntervalMs < 0) {
- throw new AnalysisException("group_commit_interval_ms can
not be less than 0:"
- + groupCommitIntervalMsStr);
- }
- } catch (NumberFormatException e) {
- throw new AnalysisException("Invalid group_commit_interval_ms
format: "
- + groupCommitIntervalMsStr);
- }
+ PropertyAnalyzer.analyzeGroupCommitIntervalMs(properties, false);
this.needTableStable = false;
this.opType = AlterOpType.MODIFY_TABLE_PROPERTY_SYNC;
} else if
(properties.containsKey(PropertyAnalyzer.PROPERTIES_GROUP_COMMIT_DATA_BYTES)) {
- long groupCommitDataBytes;
- String groupCommitDataBytesStr =
properties.get(PropertyAnalyzer.PROPERTIES_GROUP_COMMIT_DATA_BYTES);
- try {
- groupCommitDataBytes = Long.parseLong(groupCommitDataBytesStr);
- if (groupCommitDataBytes < 0) {
- throw new AnalysisException("group_commit_data_bytes can
not be less than 0:"
- + groupCommitDataBytesStr);
- }
- } catch (NumberFormatException e) {
- throw new AnalysisException("Invalid group_commit_data_bytes
format: "
- + groupCommitDataBytesStr);
- }
+ PropertyAnalyzer.analyzeGroupCommitDataBytes(properties, false);
this.needTableStable = false;
Review Comment:
This change delegates validation to PropertyAnalyzer, but it also changes
the user-facing error text from the previous, more specific messages (which
included the invalid value). If keeping centralized parsing, consider
preserving the previous level of detail in the thrown message (or wrapping with
context here) to avoid regressing diagnosability.
--
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]
