Repository navigation
Conversation
pan3793
changed the title
There was a problem hiding this comment.
Thanks for the backport! Reviewed against branch-3.5. One structural issue; the code itself looks good.
This PR bundles a commit that is already on branch-3.5. There are two commits here:
cfeb182-- "Improve handling of fetches with null and non-null stream ids": changesOneForOneStreamManagerand three network-common test suites.4182b12-- "[MINOR][CORE] Avoid accidental collisions in SHS using LevelDB": the KVStore change the PR body describes.
The first commit is already merged to branch-3.5 as 3551b280 (same commit message), and OneForOneStreamManager.java is byte-for-byte identical between this PR's head and the current branch-3.5 tip. GitHub's compare reports the branch as diverged (ahead 2 / behind 2). The large network-common diff shown by gh pr diff is only an artifact of diffing against the older merge-base rather than the current branch tip.
Could you rebase onto the latest branch-3.5? The duplicate network commit will drop out, leaving only the KVStore change, which matches the title and description. As-is, the network portion of the diff adds nothing over what is already on the branch and makes the PR harder to read.
The KVStore change itself LGTM. 4182b12 is a faithful backport of master's 23fdef7 (same four files, +9 lines each). The key-separator (0x00) check is correct: getBytes(UTF_8) emits a 0x00 byte only for the U+0000 character, and the array key path routes each element back through the same String branch, so the test case that passes a String array whose element contains an embedded 0x00 is genuinely exercised. Failing fast is the right behavior, since such values were silently corrupting index keys before.
nit: the description covers only the KVStore backport; please also mention (or, after the rebase, drop) the network change so the PR body matches its contents.
4182b12 to
9b8f3c2
Compare
34f0e91 to
825f887
Compare
String index values are copied verbatim into store keys, so an embedded key separator (0x00) corrupts the index key structure and mixes entries across range scans and deletes. Fail fast with IllegalArgumentException in LevelDBTypeInfo and RocksDBTypeInfo instead. Co-authored-by: Cursor <cursoragent@cursor.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Holden Karau <holden@pigscanfly.ca>
825f887 to
7998c08
Compare
String index values are copied verbatim into store keys, so an embedded key separator (0x00) corrupts the index key structure and mixes entries across range scans and deletes. Fail fast with IllegalArgumentException in LevelDBTypeInfo and RocksDBTypeInfo instead. This is a backport of 23fdef7 Closes #58616 from holdenk/111-kvstore-index-validation-branch-3.5. Lead-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Signed-off-by: Holden Karau <holden.karau@snowflake.com>
|
Merged to 3.5 |
String index values are copied verbatim into store keys, so an embedded key separator (0x00) corrupts the index key structure and mixes entries across range scans and deletes. Fail fast with IllegalArgumentException in LevelDBTypeInfo and RocksDBTypeInfo instead.
This is a backport of 23fdef7