-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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-47746] Implement ordinal-based range encoding in the RocksDBStateEncoder #45905
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only nits.
private val rangeScanKeyFieldsWithIdx: Seq[(StructField, Int)] = { | ||
keySchema.zipWithIndex.take(numOrderingCols) | ||
private val rangeScanKeyFieldsWithOrdinal: Seq[(StructField, Int)] = { | ||
orderingOrdinals.map(ordinal => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private val remainingKeyFieldsWithIdx: Seq[(StructField, Int)] = { | ||
keySchema.zipWithIndex.drop(numOrderingCols) | ||
private val remainingKeyFieldsWithOrdinal: Seq[(StructField, Int)] = { | ||
0.to(keySchema.length - 1).diff(orderingOrdinals).map(ordinal => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same, { ordinal =>
assert(valueRowToData(store.get(keyRow, cfName)) === 1) | ||
} | ||
|
||
// scalastyle:off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you had to disable style? when you disable style for valid reason, please be specific to disable a single rule, and also make sure to re-enable as long as it doesn't need to be disabled further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a typo!
f04801d
to
f6efa6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 pending CI
Thanks! Merging to master. |
What changes were proposed in this pull request?
The RocksDBStateEncoder now implements range projection by reading a list of ordering ordinals, and using that to project certain columns, in big-endian, to the front of the
Array[Byte]
encoded rows returned by the encoder.Why are the changes needed?
StateV2 implementations (and other state-related operators) project certain columns to the front of
UnsafeRow
s, and then rely on the RocksDBStateEncoder to range-encode those columns. We can avoid the initial projection by just passing the RocksDBStateEncoder the ordinals to encode at the front. This should avoid any GC or codegen overheads associated with projection.Does this PR introduce any user-facing change?
No.
How was this patch tested?
New UTs. All existing UTs should pass.
Was this patch authored or co-authored using generative AI tooling?
Yes