Conversation
csun5285
requested review from
airborne12,
deardeng,
eldenmoon,
gavinchou,
liaoxin01,
luwei16 and
yiguolei
as code owners
September 18, 2026 09:45
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
`StorageReadOptions::tablet_schema` carried a whole `TabletSchema` down to every segment iterator, but it served two unrelated roles: - tablet-level facts the segment already knows (keys type, index storage format, whether a column is a key), and - the compaction *output* schema, which genuinely differs from the segment's own schema and is what variant compaction needs to lay out its subcolumns. Conflating them made the schema look like a general read input, so callers that only ever read on the query path had to produce one, and code that needed the compaction output schema looked like it worked on any read. This splits the two and deletes the field: 1. `StorageReadOptions::tablet_schema` is gone. Tablet-level facts are read from `Segment::_tablet_schema`, and the two facts a read cannot recover from the segment -- whether the tablet defines a sequence mapping, and whether it materializes variant subcolumns as extracted columns -- move onto `ReadSchema` as `tablet_has_sequence_map()` and `tablet_has_extracted_variant_columns()`. 2. `TabletSchema::_path_set_info_map` moves out into a new `VariantCompactionPaths` (`be/src/storage/variant_compaction_paths.h`). It was only ever built by compaction and only ever read by a compaction reader and writer, yet it lived on the schema, where it was never persisted and never survived a `RowsetMeta::set_tablet_schema` round trip. Living on the schema made `TabletSchema::inverted_indexs()` look like it needed it everywhere; it did not. The layout now travels explicitly through `RowsetWriterContext` / `TabletReaderParams` / `StorageReadOptions` as a `shared_ptr<const ...>`, shared read-only by the one compaction that built it. 3. `ReadSchema`'s three public init methods collapse into a single `init_from_tablet_schema(schema, merge_by_sequence_mapping, map_row_binlog_columns)`, and `RowsetReaderContext::tablet_schema` is deleted. The two flags are decided by `BlockReader` / `VerticalBlockReader`, which is where the reader type, direct mode and MoW-ness are known. 4. `Segment::seek_and_read_by_rowid` takes the `TabletColumn` it reads instead of a whole `TabletSchema`, which is all it used. Its two callers (`PointQueryExecutor`, `RowIdFetcher`) resolve the column once and pass it in. 5. `OrderedDataCompactionTest` and `SegmentsKeyBoundsTruncationTest` now restore `ordered_data_compaction_min_segment_size`. Both leaked it, which sent every later suite's compaction down the ordered link-file path instead of a real merge -- this is what made `IndexStorageVariant*` fail depending on suite order. No behavior change intended on the query path. Tests: `be/test/storage/variant_compaction_paths_test.cpp` is new and ports the coverage that was deleted with `_path_set_info_map`; `read_schema_test.cpp` gains the sequence-map and extracted-variant cases; `segcompaction_test.cpp` gains `VariantRowsetIsNeverSegmentCompacted`, pinning the existing rule that a rowset with a variant column is never segment-compacted.
csun5285
force-pushed
the
rm-read-options-tablet-schema
branch
from
September 18, 2026 09:52
d134760 to
56c2d55
Compare
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
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 summary
Outcome: changes requested. The production ownership refactor is coherent in the reviewed paths, but the new segcompaction regression test has one deterministic-performance and scheduler-flakiness issue that should be fixed. This review covers exact head 56c2d553e6864c95c4f3a12180ad5ef5990a9e28; no additional user focus was supplied.
Critical checkpoint conclusions
- Goal and scope: The change removes
TabletSchemafrom query read options, moves projection-independent facts intoReadSchema, externalizes transient Variant compaction layouts, and moves rowid column resolution to callers. The 66 changed files and their upstream/downstream call chains were covered; the implementation remains focused on that ownership split. - Correctness and parallel paths: Query, checksum, row-binlog, local/cloud schema change, local/cloud horizontal and vertical compaction, cold/ordered compaction, segcompaction, index build, point query, and rowid fetch were traced.
ReadSchemafacts are initialized before reader capture, and physical segment facts remain sourced from the rowset schema. No production wrong-row or compatibility regression survived review. - Concurrency: Reader-schema mutation is single-owner setup followed by read-only use. The Variant layout is published as immutable shared state and retained through every consuming reader/writer lifetime; no new lock order or mutable-data race was found. The accepted inline issue is specifically the test's nondeterministic coordination with the asynchronous segcompaction worker.
- Lifecycle and static initialization: Shared ownership covers segment, iterator, layout, and raw index-metadata lifetimes through teardown. No ownership cycle, dangling reference, or cross-translation-unit static initialization dependency was introduced.
- Configuration: No configuration item was added. Changed tests restore the existing globals they modify, aside from the accepted wall-clock synchronization problem.
- Compatibility and persistence: Internal signature changes have migrated call sites. No FE-BE field, wire protocol, EditLog, rowset format, or rolling-upgrade contract was added or changed; legacy rowset-schema fallback behavior is retained.
- Conditions, errors, and observability: New
Statusresults are checked and impossible layout states fail through existing invariants. Existing compaction, reader, and index diagnostics are adequate; no new metric or log is required for this refactor. - Transactions and data writes: Rowset visibility, commit-TSO handling, compaction write atomicity, and local/cloud writer behavior remain unchanged in the traced flows. No new failover or transaction-state boundary was introduced.
- Performance: No production hot-path regression was identified. The test helper always adds about 12 seconds and still permits a scheduler-dependent false failure; see the inline comment.
- Tests and validation: The patch adds or migrates unit coverage for schema facts, commit TSO, count/no-read gates, Variant layout/index behavior, rowid paths, and compaction helpers. Per the review contract, this was static analysis only: I ran no build, unit test, regression test, or source modification. Current GitHub status shows formatting/checkstyle/license/secret checks passing, while broad build/test jobs are skipped; author/CI claims are not treated as independent runtime validation.
- Completion: Three bounded rounds completed. Both full-coverage passes and the separate risk-focused pass in the final round returned
NO_NEW_VALUABLE_FINDINGS; all candidates were independently adjudicated and the review is complete, not capped/incomplete.
| ASSERT_TRUE(add_block_with_columns(rowset_writer.get(), &block, &columns).ok()); | ||
| ASSERT_TRUE(rowset_writer->flush().ok()); | ||
| if (wait_for_segcompaction) { | ||
| sleep(1); |
Contributor
There was a problem hiding this comment.
[P2] Synchronize with segcompaction instead of sleeping
This helper sleeps after all 12 flushes, so the new test adds roughly 12 seconds even though no task can be submitted before the fifth segment. It is still timing-dependent: build() cancels a queued task if the worker has not claimed it yet, so a busy runner can leave the control rowset unmerged and fail EXPECT_LT even when segcompaction is correct. Please wait on a sync point/condition that proves the worker started or completed instead of using wall-clock sleeps.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
StorageReadOptions::tablet_schemais gone. Tablet-level facts are read fromSegment::_tablet_schema, and the two facts a read cannot recover from the segment -- whether the tablet defines a sequence mapping, and whether it materializes variant subcolumns as extracted columns -- move ontoReadSchemaastablet_has_sequence_map()andtablet_has_extracted_variant_columns().TabletSchema::_path_set_info_mapmoves out into a newVariantCompactionPaths(be/src/storage/variant_compaction_paths.h). It was only ever built by compaction and only ever read by a compaction reader and writer, yet it lived on the schema, where it was never persisted and never survived aRowsetMeta::set_tablet_schemaround trip. Living on the schema madeTabletSchema::inverted_indexs()look like it needed it everywhere; it did not. The layout now travels explicitly throughRowsetWriterContext/TabletReaderParams/StorageReadOptionsas ashared_ptr<const ...>, shared read-only by the one compaction that built it.ReadSchema's three public init methods collapse into a singleinit_from_tablet_schema(schema, merge_by_sequence_mapping, map_row_binlog_columns), andRowsetReaderContext::tablet_schemais deleted. The two flags are decided byBlockReader/VerticalBlockReader, which is where the reader type, direct mode and MoW-ness are known.Segment::seek_and_read_by_rowidtakes theTabletColumnit reads instead of a wholeTabletSchema, which is all it used. Its two callers (PointQueryExecutor,RowIdFetcher) resolve the column once and pass it in.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)