Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the SingleShard sharding strategy’s shardStats() behavior to match the canonical “array of stats” shape and removes any usage in the affected code paths, aligning with the stricter static analysis work from #922.
Changes:
- Add a
ShardStatstype and use it as the return type forShardStore.shardStats. - Fix
SingleShard.shardStats()to return an array of stats (and use stringshardId) instead of a single object withany. - Add a regression test ensuring
SingleShard.shardStats()returns the canonical array shape.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/test/sharding.test.ts | Adds a regression test for SingleShard.shardStats() returning the canonical array shape. |
| src/internal/sharding/strategy/single-shard.ts | Updates SingleShard.shardStats() to return ShardStats and removes the unused shardStatsByKind() method. |
| src/internal/sharding/store.ts | Introduces ShardStats type alias and applies it to ShardStore.shardStats() return type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage Report for CI Build 24131510062Coverage increased (+0.02%) to 80.817%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
260fc31 to
faf2a59
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/internal/sharding/strategy/single-shard.ts:42
SingleShard.withTnxdoesn't match theSharderinterface signature (withTnx(tnx: unknown): Sharder). Even if the transaction is unused for the single-shard strategy, the method should accept thetnxparameter (and typically returnthisor a new instance) to satisfy the contract and avoid type errors at call sites.
withTnx(): Sharder {
return new SingleShard({
shardKey: this.singleShard.shardKey,
capacity: this.singleShard.capacity,
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
faf2a59 to
78e0d7e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/internal/sharding/strategy/single-shard.ts:22
listShardByKindignores the requestedkindand always returns a shard row withkind: 'iceberg-table'. SinceSingleShardis also used for non-iceberg sharding (e.g., vector), this can return incorrect data if the method is used for other kinds. Consider returningkind: _kind(or otherwise deriving the kind from the input) so the method behaves consistently with its signature.
listShardByKind(_kind: ResourceKind): Promise<ShardRow[]> {
return Promise.resolve([
{
id: 1,
kind: 'iceberg-table',
shard_key: this.singleShard.shardKey,
capacity: this.singleShard.capacity,
next_slot: -1,
status: 'active',
created_at: new Date().toISOString(),
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
78e0d7e to
3034e41
Compare
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
3034e41 to
1d2f7cf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/internal/sharding/strategy/single-shard.ts:17
listShardByKindignores the requestedkindand always returns a shard row withkind: 'iceberg-table'. This makes the method semantically incorrect for callers asking for'vector'(or any future kind), and can lead to misleading results. Consider using the_kindparameter to set the returned row’skind(and/or returning an empty array when the requested kind doesn’t match what the single-shard instance is intended to represent).
listShardByKind(_kind: ResourceKind): Promise<ShardRow[]> {
return Promise.resolve([
{
id: 1,
kind: 'iceberg-table',
shard_key: this.singleShard.shardKey,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Single shard doesn't implement sharder interface correctly, missed due to any usage.
What is the new behavior?
Fix the type and drop any.
Additional context
Related to #922, extracted because of behavior change.