Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support prefix scans in ResponsiveKeyValueStore #411

Merged
merged 4 commits into from
Jan 14, 2025
Merged

support prefix scans in ResponsiveKeyValueStore #411

merged 4 commits into from
Jan 14, 2025

Conversation

agavra
Copy link
Contributor

@agavra agavra commented Jan 13, 2025

fixes #408

Support prefix scans on:

  • CassandraKeyValueTable
  • MongoKeyValueTable
  • InMemoryKeyValueTable

We don't support range scans on Global tables (yet), CassandraFactTable or RS3KeyValueTable yet.

@agavra agavra requested review from ableegoldman and rodesai and removed request for ableegoldman January 13, 2025 22:47
) {
final Bytes from = Bytes.wrap(prefixKeySerializer.serialize(null, prefix));
final Bytes to = Utils.incrementWithoutOverflow(from);
return doRange(kafkaPartition, from, to, streamTimeMs, this.prefix);
Copy link
Contributor

Choose a reason for hiding this comment

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

very nitpicky but why use range above but this.prefix here? Can only afford one this ? 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😆 it's because this has prefix in the parameters so there's a scoping conflict whereas range doesn't have that. I'll use this for both for symmetry if you demand it haha

try {
return Bytes.increment(input);
} catch (final IndexOutOfBoundsException e) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the semantics of an upper bound of null in Mongo and Scylla? Semantically it should just not apply any upper bound at all, is that what they both do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, fixed bugs and added tests.

@@ -196,6 +197,40 @@ public void shouldReturnRangeKeysInLexicalOrderAcrossMultipleSubPartitions() {
}
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test that hits the overflow case? Would be good to have one for each table type to make sure the underlying engine does the right thing

@agavra agavra merged commit 331cbc7 into main Jan 14, 2025
1 check passed
@agavra agavra deleted the prefix_scans branch January 14, 2025 18:52
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.

Support prefixScan on ResponsiveKeyValueStore
2 participants