Skip to content

fix: stats for single shard#992

Merged
ferhatelmas merged 1 commit intomasterfrom
ferhat/shard-stats
Apr 8, 2026
Merged

fix: stats for single shard#992
ferhatelmas merged 1 commit intomasterfrom
ferhat/shard-stats

Conversation

@ferhatelmas
Copy link
Copy Markdown
Member

@ferhatelmas ferhatelmas commented Apr 8, 2026

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.

@ferhatelmas ferhatelmas requested a review from a team as a code owner April 8, 2026 10:11
Copilot AI review requested due to automatic review settings April 8, 2026 10:11
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ShardStats type and use it as the return type for ShardStore.shardStats.
  • Fix SingleShard.shardStats() to return an array of stats (and use string shardId) instead of a single object with any.
  • 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.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 8, 2026

Coverage Report for CI Build 24131510062

Coverage increased (+0.02%) to 80.817%

Details

  • Coverage increased (+0.02%) from the base build.
  • Patch coverage: 41 of 41 lines across 3 files are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 37220
Covered Lines: 30260
Line Coverage: 81.3%
Relevant Branches: 4134
Covered Branches: 3161
Branch Coverage: 76.46%
Branches in Coverage %: Yes
Coverage Strength: 317.08 hits per line

💛 - Coveralls

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.withTnx doesn't match the Sharder interface signature (withTnx(tnx: unknown): Sharder). Even if the transaction is unused for the single-shard strategy, the method should accept the tnx parameter (and typically return this or 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.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • listShardByKind ignores the requested kind and always returns a shard row with kind: 'iceberg-table'. Since SingleShard is also used for non-iceberg sharding (e.g., vector), this can return incorrect data if the method is used for other kinds. Consider returning kind: _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.

Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • listShardByKind ignores the requested kind and always returns a shard row with kind: '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 _kind parameter to set the returned row’s kind (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.

@ferhatelmas ferhatelmas merged commit f6e193a into master Apr 8, 2026
9 checks passed
@ferhatelmas ferhatelmas deleted the ferhat/shard-stats branch April 8, 2026 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants