[refactor](storage) extract RowKeyEncoder: dedup key encoding across both segment writers#65492
Open
csun5285 wants to merge 2 commits into
Open
[refactor](storage) extract RowKeyEncoder: dedup key encoding across both segment writers#65492csun5285 wants to merge 2 commits into
csun5285 wants to merge 2 commits into
Conversation
…both segment writers
SegmentWriter and VerticalSegmentWriter carry two verbatim copies of the
row-key encoding logic (_full_encode_keys x2, _encode_seq_column,
_encode_rowid, _encode_keys, plus six coder members each). Extract it into
a shared RowKeyEncoder (be/src/storage/key/row_key_encoder.{h,cpp}) with
the two views made explicit:
- full_encode() / encode_short_keys(): the sort-key view (cluster key
columns for mow tables with cluster keys, schema key columns otherwise)
- full_encode_primary_keys(): the primary-key view (always the schema key
columns; the primary key index is built on these even when the segment
sorts by cluster keys)
- append_seq_suffix() / append_rowid_suffix(): the two key suffixes
Both writers now hold a RowKeyEncoder member and _generate_primary_key_index
loses its coder parameter. BlockAggregator's calls into the writer's encode
helpers in partial_update_info.cpp are redirected onto the writer's
_key_encoder (decoupling BlockAggregator from the writer entirely is left
to a follow-up).
Equivalence notes for review:
- SegmentWriter's _full_encode_keys had a null_first parameter that no
caller ever set to false; the encoder always uses the null-first marker.
- The constructor DCHECK(num_sort_key_columns >= num_short_key_columns) is
dropped; both counts now derive from TabletSchema in one place.
- SegmentWriter::set_mow_context had zero callers and is removed.
- _maybe_invalid_row_cache intentionally stays in the writers; moving the
row-cache invalidation is out of scope here.
Tests: row_key_encoder_test.cpp with 30 cases - 13 behavior cases (marker
layout, null-first ordering, sort-key vs primary-key views, short-key
truncation and collision, seq/rowid suffixes) plus a golden byte matrix
that pins the encoded bytes of every legal key-column type (TINYINT,
SMALLINT, INT, BIGINT, LARGEINT, BOOL, DATEV2, DATETIMEV2, DECIMAL32/64/
128I/256, IPV4, IPV6, CHAR, VARCHAR, STRING) in all three views as literal
hex strings, so any coder change or CPU-architecture drift fails loudly.
Every existing MoW / partial-update / pk-index UT now executes through
RowKeyEncoder.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
csun5285
requested review from
airborne12,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review completed with one inline issue.
Critical checkpoint conclusions:
- Goal/refactor: the RowKeyEncoder extraction preserves the old writer-local full-key, short-key, sequence-suffix, cluster-key primary-key separation, and rowid-suffix flows based on static comparison with GitHub base snapshots.
- Scope: the production changes are focused in BE storage key/segment writer paths; I did not find another substantiated production correctness issue.
- Concurrency/lifecycle: no new shared mutable state, thread entry, lock ordering, or special lifetime boundary was introduced by the helper; it stores schema-derived coder pointers like the old writers did.
- Config/compatibility/persistence: no config, protocol, edit-log, or storage-format change was found; the encoded byte layout is covered by new unit-test goldens.
- Parallel paths: SegmentWriter, VerticalSegmentWriter, and BlockAggregator call paths were checked, including cluster-key and sequence-column paths.
- Tests/style: one test hygiene issue remains: the new unit test leaks
private/protectedmacros into later includes. - User focus: no additional user-provided focus was present.
Validation was static only: this checkout is missing .worktree_initialized, thirdparty/installed, thirdparty/installed/bin/protoc, and clang-format v16, and the base commit is not present locally. I used GitHub PR metadata/base snapshots for diff scope. git show --check reported no whitespace errors on the changed files.
Subagent conclusions: tests-session-config proposed TSC-001, accepted as MAIN-001 and submitted inline. optimizer-rewrite found no candidates. Convergence round 1 ended with both live subagents returning NO_NEW_VALUABLE_FINDINGS for the final one-comment ledger set.
… only Address review: the white-box macros were defined before every include of the test TU, so gtest, standard and other Doris headers were all parsed with rewritten access specifiers. Wrap only storage/tablet/tablet_schema.h (the one header the test needs white-box access to) and #undef right after, following the convention of segment_iterator_limit_opt_test.cpp. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary:
I found one test-coverage issue in the new RowKeyEncoder unit test. The production refactor itself looks byte-compatible with the removed SegmentWriter/VerticalSegmentWriter helper logic after checking full-key, short-key, null, sequence, rowid, cluster-key MoW, partial-update reachability, min/max, and lookup-side consumers.
Critical checkpoint conclusions:
- Goal/test proof: the refactor has focused unit coverage for marker layout, suffixes, short keys, and many key coders, but the cluster-key separation test does not prove the primary-vs-sort coder split.
- Scope: the implementation is focused on extracting key encoding into RowKeyEncoder and replacing the writer-local helpers.
- Concurrency/lifecycle/config/persistence/FE-BE protocol: no new concurrency, dynamic config, persistence format, or protocol surface was introduced.
- Parallel paths: SegmentWriter and VerticalSegmentWriter were both updated; partial-update and lookup-side paths were checked for compatibility.
- Data correctness: no production key byte-layout or accessor-routing bug was substantiated.
- Tests: one targeted test needs strengthening; no build/tests were run because the review prompt explicitly restricts this task to review operations.
- User focus: no additional user-provided review focus was supplied.
|
|
||
| // With cluster keys the segment sorts by the cluster key columns, while the | ||
| // primary key index is still built over the schema key columns. | ||
| TEST_F(RowKeyEncoderTest, ClusterKeySortDiffersFromPrimaryKeys) { |
Contributor
There was a problem hiding this comment.
This test does not actually catch the primary-vs-cluster coder mixup that the refactor is trying to protect against. cluster_key_schema() makes both the primary key and the cluster key INT columns, and this assertion only proves that encoded value 5 differs from encoded value 99. If full_encode_primary_keys() accidentally used the cluster/sort-key coder vector, the test would still pass because the supplied primary-key accessor is also INT. Please make this case heterogeneous, for example INT primary key plus CHAR/VARCHAR cluster key, and assert the exact hex for full_encode_primary_keys(primary_keys), full_encode(sort_keys), and the short key so a wrong coder vector fails deterministically.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29969 ms |
Contributor
TPC-DS: Total hot run time: 180905 ms |
Contributor
ClickBench: Total hot run time: 25.22 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both writers now hold a RowKeyEncoder member and _generate_primary_key_index loses its coder parameter. BlockAggregator's calls into the writer's encode helpers in partial_update_info.cpp are redirected onto the writer's _key_encoder (decoupling BlockAggregator from the writer entirely is left to a follow-up).
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)