github-actions[bot] commented on code in PR #68195:
URL: https://github.com/apache/doris/pull/68195#discussion_r4091088054
##########
fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java:
##########
@@ -904,6 +905,12 @@ public List<Column>
checkAndPrepareMaterializedView(AddRollupOp addRollupOp, Ola
throw new DdlException("Invalid column order. key
should before all values: " + rollupColName);
}
+ // BE sorts rollup rows by the duplicate keys, which these
types cannot be compared on.
+ if (isKey && baseColumn.getType().isOnlyMetricType()) {
+ throw new DdlException("Column[" + rollupColName + "]
can not be used as a duplicate key "
Review Comment:
[P1] Roll back colocation registration on this CREATE failure
Inline rollups reach this throw from `InternalCatalog.createOlapTable` only
after `ColocateTableIndex.addTableToGroup(...)` has inserted the new table
ID/group schema, while the cleanup `try/catch` that calls
`removeTable(tableId)` starts later around partition creation. For example, use
base columns `(id INT, v VARIANT, s INT)`, rollup `(s, v, id) DUPLICATE KEY(s,
v)`, and `"colocate_with"="g"`: pre-patch short-key calculation accepts `s` and
reaches that cleanup scope, but this new check now throws on `v` before it. The
failed CREATE therefore leaves a phantom table/group, so a corrected retry can
be rejected against leaked schema and background code can observe a nonexistent
table ID. Please validate inline rollups before registering colocation
metadata, or widen cleanup to cover this exception path.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java:
##########
@@ -904,6 +905,12 @@ public List<Column>
checkAndPrepareMaterializedView(AddRollupOp addRollupOp, Ola
throw new DdlException("Invalid column order. key
should before all values: " + rollupColName);
}
+ // BE sorts rollup rows by the duplicate keys, which these
types cannot be compared on.
+ if (isKey && baseColumn.getType().isOnlyMetricType()) {
Review Comment:
[P1] Include AGG_STATE in the non-key type boundary
`Type.isOnlyMetricType()` does not include catalog `AggStateType`. This type
is reachable on a DUP_KEYS table by enabling agg state and running `ADD COLUMN
st AGG_STATE<sum(INT NOT NULL)> GENERIC NULL`: `AddColumnOp` validates
`GENERIC`, then `SchemaChangeHandler.addColumnInternal` rewrites the non-key
aggregation to `NONE` without revalidating it. A later `ADD ROLLUP ...
DUPLICATE KEY(st)` therefore passes this check and promotes `st`; the
inferred-key branch also promotes it because `Type.couldBeShortKey()` omits
AggState. BE has no AGG_STATE key coder, and fixed-length states use throwing
compare/accessor paths, so the rollup still fails asynchronously. Please
include AggState in the canonical non-key eligibility used by both branches and
add this regression.
--
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]
