Skip to content

feat: check if tuples belongs to same shard before rewriting#1065

Merged
levkk merged 2 commits into
pgdogdev:mainfrom
murex971:nagrawal-shard-check
Jun 12, 2026
Merged

feat: check if tuples belongs to same shard before rewriting#1065
levkk merged 2 commits into
pgdogdev:mainfrom
murex971:nagrawal-shard-check

Conversation

@murex971

@murex971 murex971 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

fixes #1012

Adds uniform_shard() helper that iterates the split requests and returns Some(n) only if every split has Shard::Direct(n) for the same n, otherwise None.

Testing

added and ran following unit test:
cargo nextest run --profile dev -E 'test(same_shard_insert) | test(cross_shard_insert)'

@CLAassistant

CLAassistant commented Jun 12, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@levkk levkk requested a review from sgrif June 12, 2026 13:21
Comment thread integration/rust/tests/integration/rewrite.rs Outdated
Comment thread integration/rust/tests/integration/rewrite.rs Outdated
Comment thread integration/rust/tests/integration/rewrite.rs Outdated
Comment thread pgdog/src/frontend/client/query_engine/multi_step/insert.rs Outdated
@sgrif

sgrif commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

It would also be nice to have a unit test for this in addition to the integration tests

@levkk

levkk commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Agreed. The integration test doesn't assert the changed behavior - it will pass in both scenarios (if the rewrite happened or not).

@sgrif

sgrif commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Yeah, now that I think about it I'd just delete the integration test. This is essentially an implementation detail and not something that can be meaningfully tested at the client level

@murex971

Copy link
Copy Markdown
Contributor Author

@sgrif @levkk removed the integration test in favour of unit tests, resolved other comments as well, thanks!

@murex971 murex971 requested a review from sgrif June 12, 2026 19:22

@sgrif sgrif left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't see anything worth blocking this over

@levkk levkk merged commit 5dd42c3 into pgdogdev:main Jun 12, 2026
24 checks passed
@levkk levkk changed the title feat: check if tuples belongs to same shard before inserting feat: check if tuples belongs to same shard before rewriting Jun 12, 2026
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.

[Rewrite] Check INSERT tuples belong on different shards before rewriting

4 participants