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

[SPARK-46979][SS] Add support for specifying key and value encoder separately and also for each col family in RocksDB state store provider #45038

Closed
wants to merge 6 commits into from

Conversation

anishshri-db
Copy link
Contributor

@anishshri-db anishshri-db commented Feb 6, 2024

What changes were proposed in this pull request?

Add support for specifying key and value encoder separately and also for each col family in RocksDB state store provider

Why are the changes needed?

This change allows us to specify encoder for key/values separately and avoid encoding additional bytes. Also, it allows us to set schemas/encoders for individual column families, which will be required for future changes related to transformWithState operator (listState/timer changes etc)

We are refactoring a bit here given the upcoming changes. so we are proposing to split key and value encoders.
Key encoders can be of 2 types:

  • with prefix scan
  • without prefix scan

Value encoders can also eventually be of 2 types:

  • single value
  • multiple values (used for list state)

And we now also allow setting schema and getting encoder for each column family.
So after the change, we can potentially allow something like this:

  • col family 1 - with keySchema with prefix scan and valueSchema with single value and binary type
  • col family 2 - with keySchema without prefix scan and valueSchema with multiple values

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added unit tests

[info] Run completed in 3 minutes, 5 seconds.
[info] Total number of tests run: 286
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 286, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.

Was this patch authored or co-authored using generative AI tooling?

No

…tely and also for each col family in RocksDB state store provider
@anishshri-db anishshri-db changed the title [SPARK-46979] Add support for specifying key and value encoder separately and also for each col family in RocksDB state store provider [SPARK-46979][SS] Add support for specifying key and value encoder separately and also for each col family in RocksDB state store provider Feb 6, 2024
@anishshri-db
Copy link
Contributor Author

@HeartSaVioR - PTAL, thx !

Copy link
Contributor

@sahnib sahnib left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for separating the encoders, this helps us avoid evolve the key/value encoders independently and use both Multi valued, and prefix key encoder.

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

Only minors. Looks great in overall.

@@ -215,7 +240,9 @@ private[sql] class RocksDBStateStoreProvider
(keySchema.length > numColsPrefixKey), "The number of columns in the key must be " +
"greater than the number of columns for prefix key!")

this.encoder = RocksDBStateEncoder.getEncoder(keySchema, valueSchema, numColsPrefixKey)
keyValueEncoderMap.putIfAbsent(StateStore.DEFAULT_COL_FAMILY_NAME,
Copy link
Contributor

Choose a reason for hiding this comment

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

(Maybe microbenchmark could tell that this could regress for default column family only - map lookup with carefully crafted lock operation in every op, though I'd rather not concern before we see actual regression.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I didn't worry about it too much, given that the provider init likely happens once for long lived queries and where we can retain the use of the same provider on the same executor across m/batch executions.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, what I meant is to look up concurrent map per "every op" to figure out encoder, for existing stateful operators - previously it was just a reference to the field. But ops is relatively very cheap compared to commit as of now, so let's see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok - yea mainly didn't want to maintain 2 data structures for this. But if we find that its more expensive, then we can just split some of the logic for the default col family case

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

+1 pending CI

@HeartSaVioR
Copy link
Contributor

Thanks! Merging to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants