[refactor](storage) extract RowKeyEncoder: dedup key encoding across both segment writers#65492
[refactor](storage) extract RowKeyEncoder: dedup key encoding across both segment writers#65492csun5285 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
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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>
|
run buildall |
|
/review |
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) { |
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.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
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)