[Enhancement] Add StarOSAgent PACK shard group support and ColocateRangeUtils for range colocate#71221
[Enhancement] Add StarOSAgent PACK shard group support and ColocateRangeUtils for range colocate#71221xiangguangyxg wants to merge 1 commit intoStarRocks:mainfrom
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a7ee9a709
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fe/fe-core/src/main/java/com/starrocks/catalog/ColocateRangeUtils.java
Outdated
Show resolved
Hide resolved
…ngeUtils for range colocate Signed-off-by: xiangguangyxg <[email protected]>
xiangguangyxg
force-pushed
the
colocate_shard_group
branch
from
6a7ee9a to
cc7fc57
Compare
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 23 / 24 (95.83%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
|
There was a problem hiding this comment.
Pull request overview
This PR enhances FE support for range distribution colocate by enabling StarOS shard group creation with configurable placement policy (SPREAD/PACK), allowing shard creation into multiple shard groups, and adding a utility to expand colocate-prefix ranges into full sort-key ranges for tablet range assignment.
Changes:
- Add
StarOSAgent.createShardGroup(..., PlacementPolicy)overload and keep the existing method as a SPREAD default. - Add
StarOSAgent.createShards(..., List<Long> groupIds, ...)overload to create shards belonging to multiple shard groups. - Introduce
ColocateRangeUtils.expandToFullSortKey()plus a dedicated JUnit test suite.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| fe/fe-core/src/main/java/com/starrocks/lake/StarOSAgent.java | Adds overloads for shard-group placement policy and multi-group shard creation. |
| fe/fe-core/src/main/java/com/starrocks/catalog/ColocateRangeUtils.java | New helper to expand colocate prefix ranges into full sort-key ranges via MIN/MAX sentinels. |
| fe/fe-core/src/test/java/com/starrocks/catalog/ColocateRangeUtilsTest.java | Adds unit tests covering inclusive/exclusive and bounded/unbounded expansion cases. |
| public long createShardGroup(long dbId, long tableId, long partitionId, long indexId, | ||
| PlacementPolicy placementPolicy) throws DdlException { | ||
| prepare(); | ||
| List<ShardGroupInfo> shardGroupInfos = null; | ||
| try { | ||
| List<CreateShardGroupInfo> createShardGroupInfos = new ArrayList<>(); | ||
| createShardGroupInfos.add(CreateShardGroupInfo.newBuilder() | ||
| .setPolicy(PlacementPolicy.SPREAD) | ||
| .setPolicy(placementPolicy) | ||
| .putLabels("dbId", String.valueOf(dbId)) |
There was a problem hiding this comment.
createShardGroup(..., PlacementPolicy placementPolicy) passes placementPolicy directly into the proto builder without validation. If a caller accidentally passes null, this will fail with a NullPointerException from generated code rather than a clear DdlException/IllegalArgumentException. Consider adding an explicit null check (e.g., Preconditions/Objects.requireNonNull) and a helpful message before building the request.
| @@ -579,11 +593,13 @@ public List<Long> createShards(int numShards, FilePathInfo pathInfo, FileCacheIn | |||
|
|
|||
| CreateShardInfo.Builder builder = CreateShardInfo.newBuilder(); | |||
| builder.setReplicaCount(1) | |||
| .addGroupIds(groupId) | |||
| .setPathInfo(pathInfo) | |||
| .setCacheInfo(cacheInfo) | |||
| .putAllShardProperties(properties) | |||
| .setScheduleToWorkerGroup(workerGroupId); | |||
| for (long groupId : groupIds) { | |||
| builder.addGroupIds(groupId); | |||
| } | |||
There was a problem hiding this comment.
The new createShards(..., List<Long> groupIds, ...) overload iterates groupIds without validating it is non-null/non-empty. An empty list would create shards with no shard-group membership (likely invalid in StarOS) and a null list would NPE. Add argument validation (non-null, non-empty, and optionally reject null/duplicate IDs) before populating CreateShardInfo.
| public static Range<Tuple> expandToFullSortKey(Range<Tuple> colocateRange, | ||
| List<Column> sortKeyColumns, | ||
| int colocateColumnCount) { | ||
| if (colocateRange.isAll()) { | ||
| return Range.all(); | ||
| } | ||
| int remainingColumns = sortKeyColumns.size() - colocateColumnCount; | ||
| // Inclusive lower → extend with MIN; exclusive lower → extend with MAX | ||
| Tuple lowerBound = colocateRange.isMinimum() ? null | ||
| : extendTuple(colocateRange.getLowerBound(), sortKeyColumns, | ||
| colocateColumnCount, remainingColumns, | ||
| colocateRange.isLowerBoundIncluded()); | ||
| // Exclusive upper → extend with MIN; inclusive upper → extend with MAX |
There was a problem hiding this comment.
expandToFullSortKey does not validate colocateColumnCount relative to sortKeyColumns.size(). If colocateColumnCount is negative or greater than the sort-key length, remainingColumns becomes invalid and the method silently returns an incorrectly-shaped range. Add argument checks (0 <= colocateColumnCount <= sortKeyColumns.size()) and consider validating the provided bound tuple lengths match colocateColumnCount when the corresponding side is bounded.
| colocateRange, SORT_KEY_COLUMNS, 3); | ||
|
|
||
| // No expansion, tuple size unchanged |
There was a problem hiding this comment.
testExpandNoRemainingColumns passes colocateColumnCount = 3 (equal to the full sort-key length) but constructs bounds with makeTuple(...) which only has 1 value. The assertion then expects the expanded tuples to stay size 1, which contradicts the method’s contract of producing a full sort-key range and can mask tuple-length bugs. Consider either (a) building a 3-column tuple in the test and asserting size 3, or (b) changing the test to use colocateColumnCount = 1 if the intent is to verify the "no remaining columns" path for a 1-column sort key.
| colocateRange, SORT_KEY_COLUMNS, 3); | |
| // No expansion, tuple size unchanged | |
| colocateRange, SORT_KEY_COLUMNS.subList(0, 1), 1); | |
| // No expansion, tuple size unchanged for a single-column sort key |
Why I'm doing:
Range distribution colocate requires PACK shard groups (for co-location) and the ability
to create shards belonging to multiple shard groups simultaneously (SPREAD + PACK).
The existing StarOSAgent only supports SPREAD placement policy and single group ID.
Additionally, a utility is needed to expand colocate column ranges to full sort key ranges
for tablet range assignment.
What I'm doing:
Add
StarOSAgent.createShardGroup(PlacementPolicy)overload to support creatingshard groups with configurable placement policy (SPREAD or PACK). The existing
method delegates to the new one with SPREAD as default.
Add
StarOSAgent.createShards(List<Long> groupIds)overload to support creatingshards belonging to multiple shard groups simultaneously. The existing single-groupId
method delegates to the new one with
List.of(groupId).Add
ColocateRangeUtilshelper class withexpandToFullSortKey()method that expandsa colocate range (on colocate column prefix) to a full sort key range. The sentinel
choice (MIN or MAX) depends on bound inclusiveness to preserve semantic equivalence:
Fixes #64986
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: