github-actions[bot] commented on code in PR #61798:
URL: https://github.com/apache/doris/pull/61798#discussion_r3008425228
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/AddColumnsClause.java:
##########
@@ -19,6 +19,10 @@
import org.apache.doris.alter.AlterOpType;
import org.apache.doris.catalog.Column;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.catalog.KeysType;
Review Comment:
**Bug: Missing `setKeysType()` call + Performance: Table lookup should be
outside the loop.**
Comparing with `AddColumnClause.analyze()` (lines 76-86), there are two
problems:
1. **Missing `setKeysType()`**: `AddColumnClause` and `ModifyColumnClause`
both call `colDef.setKeysType(((OlapTable) table).getKeysType())` after the
`setIsKey()` block. Without this, `ColumnDef.keysType` remains `null`, which
affects the validation at `ColumnDef.java:361` where `keysType == null` is
treated the same as `AGG_KEYS`, potentially causing incorrect errors for
complex types in non-AGG tables.
2. **Table lookup in loop**: The table lookup is inside the `for` loop but
`tableName` doesn't change between iterations. This causes redundant DB+table
lookups for each column.
Suggested fix:
```java
if (tableName != null) {
Table table =
Env.getCurrentInternalCatalog().getDbOrAnalysisException(tableName.getDb())
.getTableOrAnalysisException(tableName.getTbl());
if (table instanceof OlapTable) {
OlapTable olapTable = (OlapTable) table;
for (ColumnDef colDef : columnDefs) {
if (olapTable.getKeysType() == KeysType.AGG_KEYS
&& colDef.getAggregateType() == null) {
colDef.setIsKey(true);
}
colDef.setKeysType(olapTable.getKeysType());
}
}
}
for (ColumnDef colDef : columnDefs) {
colDef.analyze(true);
...
}
```
Or alternatively, keep the single loop but move the table lookup before it.
--
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]
