Copilot commented on code in PR #57972:
URL: https://github.com/apache/doris/pull/57972#discussion_r2552535801
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -5697,6 +5707,99 @@ public AlterTableOp
visitReorderColumnsClause(ReorderColumnsClauseContext ctx) {
return new ReorderColumnsOp(columnsByPos, rollupName, properties);
}
+ // Helper class to reduce parameter passing
+ private static class PartitionFieldInfo {
+ final String fieldName;
+ final String transformName;
+ final Integer transformArg;
+ final String columnName;
+
+ PartitionFieldInfo(String fieldName, String transformName, Integer
transformArg, String columnName) {
+ this.fieldName = fieldName;
+ this.transformName = transformName;
+ this.transformArg = transformArg;
+ this.columnName = columnName;
+ }
+ }
+
+ private PartitionFieldInfo
extractOldPartitionFieldInfo(ReplacePartitionFieldClauseContext ctx) {
+ PartitionTransformContext oldTransformCtx = ctx.oldPartitionTransform;
+ if (oldTransformCtx == null) {
+ // old specified as identifier (appears right after KEY)
+ return new PartitionFieldInfo(ctx.getChild(3).getText(), null,
null, null);
Review Comment:
The parser method `extractOldPartitionFieldInfo()` uses a hardcoded child
index `ctx.getChild(3)` to extract the identifier when the old partition field
is specified as a name rather than a transform expression (line 5729). This
approach is fragile and error-prone:
1. It relies on the exact structure of the parse tree
2. If the grammar changes, this index could become incorrect
3. There's no validation that the child at index 3 is actually an identifier
Consider using a more robust approach, such as:
- Adding a named field in the grammar rule (e.g.,
`oldPartitionFieldName=identifier`) similar to `newPartitionFieldName`
- Or using a type-safe method to access the identifier from the context
This would make the code more maintainable and less prone to breaking when
the grammar evolves.
```suggestion
return new
PartitionFieldInfo(ctx.oldPartitionFieldName.getText(), null, null, null);
```
##########
fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:
##########
@@ -181,6 +186,16 @@ private boolean
processAlterOlapTableInternal(List<AlterClause> alterClauses, Ol
AlterOperations currentAlterOps = new AlterOperations();
currentAlterOps.checkConflict(alterClauses);
+ // Check for unsupported operations on internal tables
+ for (AlterClause clause : alterClauses) {
+ if (clause instanceof AddPartitionFieldClause) {
+ throw new UserException("ADD PARTITION KEY is only supported
for Iceberg tables");
+ }
+ if (clause instanceof DropPartitionFieldClause) {
+ throw new UserException("DROP PARTITION KEY is only supported
for Iceberg tables");
+ }
Review Comment:
The check for `ReplacePartitionFieldClause` is missing in the internal table
validation loop (lines 190-197). While `AddPartitionFieldClause` and
`DropPartitionFieldClause` are checked and rejected for internal tables,
`ReplacePartitionFieldClause` is not checked.
This could allow users to attempt a REPLACE PARTITION KEY operation on
internal tables, which would likely fail later with a less clear error message.
Add a check for `ReplacePartitionFieldClause` similar to the other two:
```java
if (clause instanceof ReplacePartitionFieldClause) {
throw new UserException("REPLACE PARTITION KEY is only supported for
Iceberg tables");
}
```
```suggestion
}
if (clause instanceof ReplacePartitionFieldClause) {
throw new UserException("REPLACE PARTITION KEY is only
supported for Iceberg tables");
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -5697,6 +5707,99 @@ public AlterTableOp
visitReorderColumnsClause(ReorderColumnsClauseContext ctx) {
return new ReorderColumnsOp(columnsByPos, rollupName, properties);
}
+ // Helper class to reduce parameter passing
+ private static class PartitionFieldInfo {
+ final String fieldName;
+ final String transformName;
+ final Integer transformArg;
+ final String columnName;
+
+ PartitionFieldInfo(String fieldName, String transformName, Integer
transformArg, String columnName) {
+ this.fieldName = fieldName;
+ this.transformName = transformName;
+ this.transformArg = transformArg;
+ this.columnName = columnName;
+ }
+ }
+
+ private PartitionFieldInfo
extractOldPartitionFieldInfo(ReplacePartitionFieldClauseContext ctx) {
+ PartitionTransformContext oldTransformCtx = ctx.oldPartitionTransform;
+ if (oldTransformCtx == null) {
+ // old specified as identifier (appears right after KEY)
+ return new PartitionFieldInfo(ctx.getChild(3).getText(), null,
null, null);
+ }
+
+ PartitionFieldInfo info = extractTransformInfo(oldTransformCtx);
+ return new PartitionFieldInfo(null, info.transformName,
info.transformArg, info.columnName);
+ }
+
+ private PartitionFieldInfo
extractNewPartitionFieldInfo(ReplacePartitionFieldClauseContext ctx) {
+ PartitionFieldInfo info =
extractTransformInfo(ctx.newPartitionTransform);
+ String partitionFieldName = ctx.newPartitionFieldName != null
+ ? ctx.newPartitionFieldName.getText()
+ : null;
+ return new PartitionFieldInfo(partitionFieldName, info.transformName,
info.transformArg, info.columnName);
+ }
+
+ private PartitionFieldInfo extractTransformInfo(PartitionTransformContext
ctx) {
+ if (ctx instanceof PartitionTransformWithArgsContext) {
+ PartitionTransformWithArgsContext argsCtx =
(PartitionTransformWithArgsContext) ctx;
+ return new PartitionFieldInfo(null,
+ argsCtx.identifier(0).getText(),
+ Integer.parseInt(argsCtx.INTEGER_VALUE().getText()),
+ argsCtx.identifier(1).getText());
+ }
+
+ if (ctx instanceof PartitionTransformWithColumnContext) {
+ PartitionTransformWithColumnContext colCtx =
(PartitionTransformWithColumnContext) ctx;
+ return new PartitionFieldInfo(null,
+ colCtx.identifier(0).getText(),
+ null,
+ colCtx.identifier(1).getText());
+ }
+
+ if (ctx instanceof PartitionTransformIdentityContext) {
+ PartitionTransformIdentityContext idCtx =
(PartitionTransformIdentityContext) ctx;
+ return new PartitionFieldInfo(null, null, null,
idCtx.identifier().getText());
+ }
+
+ return new PartitionFieldInfo(null, null, null, null);
+ }
+
+ @Override
+ public AlterTableOp
visitAddPartitionFieldClause(AddPartitionFieldClauseContext ctx) {
+ PartitionFieldInfo info =
extractTransformInfo(ctx.partitionTransform());
+ String partitionFieldName = ctx.partitionFieldName != null
+ ? ctx.partitionFieldName.getText()
+ : null;
+ return new AddPartitionFieldOp(info.transformName, info.transformArg,
+ info.columnName, partitionFieldName);
+ }
+
+ @Override
+ public AlterTableOp
visitDropPartitionFieldClause(DropPartitionFieldClauseContext ctx) {
+ PartitionTransformContext transform = ctx.partitionTransform();
+ if (transform == null) {
+ // Identifier case
+ return new DropPartitionFieldOp(ctx.getChild(ctx.getChildCount() -
1).getText());
Review Comment:
Similar issue in `visitDropPartitionFieldClause()`: the method uses
`ctx.getChild(ctx.getChildCount() - 1)` to extract the identifier (line 5784).
This is fragile because:
1. It assumes the identifier is always the last child
2. Changes to the grammar could break this assumption
3. There's no type safety or validation
Consider using a named field in the grammar rule for the identifier case,
similar to how `partitionTransform` is a named field. This would make the code
more robust and maintainable.
```suggestion
return new DropPartitionFieldOp(ctx.identifier().getText());
```
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/AddPartitionFieldClause.java:
##########
@@ -0,0 +1,108 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.analysis;
+
+import org.apache.doris.alter.AlterOpType;
+import org.apache.doris.common.UserException;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * AddPartitionFieldClause for Iceberg partition evolution
+ */
+public class AddPartitionFieldClause extends AlterTableClause {
+ private final String transformName;
+ private final Integer transformArg;
+ private final String columnName;
+ private final String partitionFieldName;
+
+ public AddPartitionFieldClause(String transformName, Integer transformArg,
String columnName,
+ String partitionFieldName) {
+ super(AlterOpType.ADD_PARTITION_FIELD);
+ this.transformName = transformName;
+ this.transformArg = transformArg;
+ this.columnName = columnName;
+ this.partitionFieldName = partitionFieldName;
+ }
+
+ public String getTransformName() {
+ return transformName;
+ }
+
+ public Integer getTransformArg() {
+ return transformArg;
+ }
+
+ public String getColumnName() {
+ return columnName;
+ }
+
+ public String getPartitionFieldName() {
+ return partitionFieldName;
+ }
+
+ @Override
+ public void analyze() throws UserException {
+ // Validation will be done in IcebergMetadataOps
+ }
+
+ @Override
+ public String toSql() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("ADD PARTITION KEY ");
+ if (transformName != null) {
+ sb.append(transformName);
+ if (transformArg != null) {
+ sb.append("(").append(transformArg);
+ if (columnName != null) {
+ sb.append(", ").append(columnName);
+ }
+ sb.append(")");
+ } else if (columnName != null) {
+ sb.append("(").append(columnName).append(")");
+ }
+ } else if (columnName != null) {
+ sb.append(columnName);
+ }
+ if (partitionFieldName != null) {
+ sb.append(" AS ").append(partitionFieldName);
+ }
+ return sb.toString();
Review Comment:
The `toSql()` method logic for building partition transform SQL is
duplicated across both the `...Op` classes (in
`nereids.trees.plans.commands.info`) and the `...Clause` classes (in
`analysis`). This creates a total of 6 instances of nearly identical code:
**Op classes:**
- AddPartitionFieldOp.toSql() (lines 91-112)
- DropPartitionFieldOp.appendPartitionTransform() (lines 110-125)
- ReplacePartitionFieldOp.appendPartitionTransform() (lines 140-156)
**Clause classes:**
- AddPartitionFieldClause.toSql() (lines 66-86)
- DropPartitionFieldClause.appendPartitionTransform() (lines 77-92)
- ReplacePartitionFieldClause.appendPartitionTransform() (lines 103-119)
Consider creating a shared utility class with a static method to handle
partition transform SQL generation to eliminate this extensive duplication.
```suggestion
return PartitionTransformSqlBuilder.buildPartitionTransformSql(
"ADD PARTITION KEY ",
transformName,
transformArg,
columnName,
partitionFieldName
);
}
// Utility for building partition transform SQL, to be shared across
classes
public static class PartitionTransformSqlBuilder {
public static String buildPartitionTransformSql(
String prefix,
String transformName,
Integer transformArg,
String columnName,
String partitionFieldName) {
StringBuilder sb = new StringBuilder();
if (prefix != null) {
sb.append(prefix);
}
if (transformName != null) {
sb.append(transformName);
if (transformArg != null) {
sb.append("(").append(transformArg);
if (columnName != null) {
sb.append(", ").append(columnName);
}
sb.append(")");
} else if (columnName != null) {
sb.append("(").append(columnName).append(")");
}
} else if (columnName != null) {
sb.append(columnName);
}
if (partitionFieldName != null) {
sb.append(" AS ").append(partitionFieldName);
}
return sb.toString();
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ReplacePartitionFieldOp.java:
##########
@@ -0,0 +1,157 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.trees.plans.commands.info;
+
+import org.apache.doris.alter.AlterOpType;
+import org.apache.doris.analysis.AlterTableClause;
+import org.apache.doris.analysis.ReplacePartitionFieldClause;
+import org.apache.doris.common.UserException;
+import org.apache.doris.qe.ConnectContext;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * ReplacePartitionFieldOp for Iceberg partition evolution
+ */
+public class ReplacePartitionFieldOp extends AlterTableOp {
+ private final String oldPartitionFieldName;
+ private final String oldTransformName;
+ private final Integer oldTransformArg;
+ private final String oldColumnName;
+ private final String newTransformName;
+ private final Integer newTransformArg;
+ private final String newColumnName;
+ private final String newPartitionFieldName;
+
+ /**
+ * Constructor for ReplacePartitionFieldOp
+ */
+ public ReplacePartitionFieldOp(String oldPartitionFieldName, String
oldTransformName,
+ Integer oldTransformArg, String oldColumnName,
+ String newTransformName, Integer newTransformArg, String
newColumnName,
+ String newPartitionFieldName) {
+ super(AlterOpType.REPLACE_PARTITION_FIELD);
+ this.oldPartitionFieldName = oldPartitionFieldName;
+ this.oldTransformName = oldTransformName;
+ this.oldTransformArg = oldTransformArg;
+ this.oldColumnName = oldColumnName;
+ this.newTransformName = newTransformName;
+ this.newTransformArg = newTransformArg;
+ this.newColumnName = newColumnName;
+ this.newPartitionFieldName = newPartitionFieldName;
+ }
+
+ public String getOldPartitionFieldName() {
+ return oldPartitionFieldName;
+ }
+
+ public String getOldTransformName() {
+ return oldTransformName;
+ }
+
+ public Integer getOldTransformArg() {
+ return oldTransformArg;
+ }
+
+ public String getOldColumnName() {
+ return oldColumnName;
+ }
+
+ public String getNewTransformName() {
+ return newTransformName;
+ }
+
+ public Integer getNewTransformArg() {
+ return newTransformArg;
+ }
+
+ public String getNewColumnName() {
+ return newColumnName;
+ }
+
+ public String getNewPartitionFieldName() {
+ return newPartitionFieldName;
+ }
+
+ @Override
+ public void validate(ConnectContext ctx) throws UserException {
+ if (oldPartitionFieldName == null && oldColumnName == null) {
+ throw new UserException("Old partition field name or old column
name must be specified");
+ }
+ if (newColumnName == null) {
+ throw new UserException("New column name must be specified");
+ }
+ }
+
+ @Override
+ public AlterTableClause translateToLegacyAlterClause() {
+ return new ReplacePartitionFieldClause(oldPartitionFieldName,
oldTransformName, oldTransformArg,
+ oldColumnName, newTransformName, newTransformArg,
newColumnName, newPartitionFieldName);
+ }
+
+ @Override
+ public Map<String, String> getProperties() {
+ return Collections.emptyMap();
+ }
+
+ @Override
+ public boolean allowOpMTMV() {
+ return false;
+ }
+
+ @Override
+ public boolean needChangeMTMVState() {
+ return false;
+ }
+
+ @Override
+ public String toSql() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("REPLACE PARTITION KEY ");
+ if (oldPartitionFieldName != null) {
+ sb.append(oldPartitionFieldName);
+ } else {
+ appendPartitionTransform(sb, oldTransformName, oldTransformArg,
oldColumnName);
+ }
+ sb.append(" WITH ");
+ appendPartitionTransform(sb, newTransformName, newTransformArg,
newColumnName);
+ if (newPartitionFieldName != null) {
+ sb.append(" AS ").append(newPartitionFieldName);
+ }
+ return sb.toString();
+ }
+
+ private void appendPartitionTransform(StringBuilder sb, String
transformName, Integer transformArg,
+ String columnName) {
+ if (transformName != null) {
+ sb.append(transformName);
+ if (transformArg != null) {
+ sb.append("(").append(transformArg);
+ if (columnName != null) {
+ sb.append(", ").append(columnName);
+ }
+ sb.append(")");
+ } else if (columnName != null) {
+ sb.append("(").append(columnName).append(")");
+ }
+ } else if (columnName != null) {
+ sb.append(columnName);
+ }
+ }
Review Comment:
The `appendPartitionTransform` method logic is duplicated across three
classes:
- `AddPartitionFieldOp.toSql()` (lines 94-107)
- `DropPartitionFieldOp.appendPartitionTransform()` (lines 110-125)
- `ReplacePartitionFieldOp.appendPartitionTransform()` (lines 140-156)
Consider extracting this common logic into a shared utility method in a base
class or helper class to improve maintainability and reduce code duplication.
--
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]
