Repository navigation
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all four changed files at 68d552b832ae557cfd2eb07c70b4375b5a62539e, plus the property/request, schema persistence, codec, writer/reader, and parallel creation paths. One P2 finding: the new FE test import violates the enforced Checkstyle order and prevents standard FE validation. No functional defect was found in the two LZ4HC mappings. There were no existing inline findings to carry forward and no additional user-provided focus.
Critical checkpoint conclusions:
- Goal and proof: both shared-nothing BE conversion and cloud FE metadata creation now preserve requested LZ4HC. Each new unit assertion reaches its changed mapping and distinguishes the old LZ4F fallback by source inspection.
- Scope and reuse: two explicit switch cases reuse the existing codec; the tests are focused. The FE import needs the inline fix.
- Concurrency and locking: mappings modify local protobuf builders before publication, adding no thread, shared state, lock acquisition, heavy locked work, or lock-order change. Existing schema-cache protection and codec context checkout/reset remain in use.
- Lifecycle and static initialization: no new static object, ownership cycle, or lifetime protocol. BE schema-cache handles and FE Mockito static mocks use the existing release/teardown paths.
- Configuration: no configuration item or dynamic-update behavior is added or changed.
- Compatibility: no new symbol, field, enum value, or storage format. LZ4HC enum 8 and its decoder already exist on release branches 3.1, 4.0, 4.1 and 4.2. Readers select codecs from persisted column/index metadata, so existing LZ4F pages retain their decoder.
- Thrift/protobuf IDs: no definition changes; the changed-definition cross-branch ID gate is inapplicable.
- Parallel paths: cloud initial rowsets, rollup/shadow/restore creation, local requests, schema copies and cloud/local conversions retain the selected codec. Normal/compaction, nested/variant and external metadata writer/reader paths propagate matching compression metadata; explicit no-compression overrides remain valid.
- Conditional logic: the added cases match adjacent enum conversions and terminate with break; no new defensive if/else, ignored Status, or failure fallback is introduced.
- Coverage: the new metadata tests target the exact regression; existing LZ4HC single/multiple-slice codec tests include empty and large inputs, and an existing suite loads/queries a table configured with LZ4HC without verifying physical codec use. No new end-to-end or upgrade test is added. Test-source validation is affected by the reported import issue.
- Expected results: LZ4HC is the correct new enum expectation; no generated result file is added or modified.
- Observability: existing metadata and codec Status/error paths cover this mapping correction; no new operation needs added metrics or logging.
- Persistence and transactions: schema serialization, cache keys and cloud conversions preserve compression_type. No EditLog, transaction, failover, visibility-version, or delete-bitmap protocol change was found.
- Data writes and atomicity: new schemas select the already implemented compressor. Segment metadata records that choice; publication, crash cleanup and rowset atomicity paths are unchanged by these cases.
- FE/BE variable passing: the existing compression_type field already reaches both builders; no new transmitted variable or missing send path.
- Performance and other checks: the added enum cases are constant-time and add no allocations or copies themselves. Selecting LZ4HC activates its existing higher CPU and page-gather buffering costs; no benchmark was run. No new header dependency, ABI requirement, or memory-accounting mechanism is introduced. No further verified issue remains.
Validation and limits: static review only. No build, Checkstyle process, unit test, regression test, runtime codec exercise, or rolling-upgrade run was performed in this invocation. The build finding is established from the changed import and the active Maven/Checkstyle configuration. Existing persisted tablet schemas are not rewritten by this patch; cloud schema-KV deduplication can retain the prior codec for an existing index/schema version. All changed paths and identified risks were reviewed; no unresolved candidate or review-scope gap remains. Review converged after three rounds.
Review converged after 3 round(s).
| import org.mockito.MockedStatic; | ||
| import org.mockito.Mockito; | ||
|
|
||
| import doris.segment_v2.SegmentV2; |
There was a problem hiding this comment.
[P2] Keep the generated-proto import in alphabetical order
doris.segment_v2.SegmentV2 is in the same third-party import group as the JUnit/Mockito imports, but it is added after them with an extra blank line. fe/check/checkstyle/checkstyle.xml enables alphabetical ordering within that group, and fe/pom.xml runs the error-level check during validate with test sources included. Consequently the standard FE Maven validation fails on this new import. Move it before org.junit.jupiter.api.AfterEach in the same import block.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #69574 at 60a6e13a62ae7b5c863b28c40a8bf8ba1eddc81c: no new actionable findings. All four changed files and the related local/cloud creation, persistence, writer and reader paths were inspected.
The existing P2 import-order finding is fixed on this head: the generated-proto import now precedes the JUnit/Mockito imports within the third-party group. No existing P0/P1 finding remains to carry forward. No additional user focus was supplied.
Critical checkpoint conclusions:
- Purpose and correctness: both metadata conversions now preserve the requested LZ4HC enum instead of selecting the LZ4F default. The added BE constructor and cloud FE builder assertions target the two missing mappings and would detect the prior fallback.
- Scope and reuse: two explicit switch cases reuse the existing enum/setters. The FE test helper accepts the codec while its existing callers retain LZ4F. The change is focused.
- Concurrency and locks: the mappings construct private schema builders before publication. They add no shared mutation, thread, lock-order edge, or blocking operation under a lock. Existing codec contexts are borrowed exclusively from a mutex-protected pool.
- Lifecycle, static initialization and memory: no new ownership, resources, static initialization or substantial allocation is introduced. Existing schema cache handles and codec memory tracking remain in place.
- Configuration: no configuration is added; existing defaults and all other codec branches are preserved.
- Compatibility: LZ4HC's persisted enum and compressor/decompressor already exist in the supplied base. Writers persist each column's codec, and readers dispatch from that file metadata, so existing LZ4F and new LZ4HC segments remain independently decodable. No symbol or storage-layout change was found.
- Thrift/protobuf evolution: no definitions, field IDs or enum values are changed; a cross-branch ID-change audit is inapplicable.
- Parallel paths: both local BE and cloud FE schema conversions are covered. Creation/partition, schema-change, rollup, restore, clone, compaction, schema copies, cloud PB conversion and V3 external/VARIANT metadata paths were traced without finding an omitted required mapping or incompatible decoder.
- Conditions and errors: the new cases follow adjacent mappings with explicit breaks. They add no speculative guards or unchecked Status/exception boundary; codec/page errors continue to propagate.
- Test coverage: new BE GoogleTest and FE JUnit 5 tests directly cover the mappings. Existing codec tests include LZ4HC single/multi-slice round trips and malformed input; existing compression suites contain LZ4HC load/query cases. These sources were inspected, not executed.
- Expected results: the new enum assertions match the requested codec. No golden-output file changes occur, and existing helper calls retain their previous inputs and cleanup.
- Observability: existing tablet-creation identifiers and codec/page failure context cover this correction; no new operation needing separate logging or metrics was identified.
- Persistence, transactions and failover: schema serialization/conversion retains compression. No EditLog, transaction-state, replay-order or master-failover protocol change is introduced.
- Data writes, atomicity and crashes: page metadata and the selected codec agree. Visibility versions, rowset publication, MoW/delete-bitmap consistency and crash-recovery ordering are unchanged; no new failure window was identified.
- FE-BE propagation: no new variable is introduced; the existing compression enum reaches both corrected endpoints through their current transport paths.
- Performance: enum selection remains constant work with no added scan/copy. The selected existing HC codec's compression cost follows the user's explicit choice.
- Other applicable checks: no new header/include, template, PCH, hygiene-table or libc dependency changes were introduced. No additional defect was substantiated after the missed-issue rechecks.
Scope limits: this is a static-only review; no builds, style tools, tests, runtime compaction or rolling-upgrade validation were run. The new tests assert schema construction, without adding a dedicated persisted-metadata or mixed-codec end-to-end test. This patch does not migrate previously persisted LZ4F schemas; cloud schema deduplication can retain an existing shared schema. The compatibility conclusion is based on the inspected head and supplied base implementations.
Convergence: two review rounds, including a final pass over this frozen payload by the BE, FE and dedicated codec-risk reviewers. No unresolved candidate or uncovered assigned static-review scope remains.
Review converged after 2 round(s).
TPC-H: Total hot run time: 28470 ms |
TPC-DS: Total hot run time: 150788 ms |
ClickBench: Total hot run time: 24.4 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: DORIS-28505
Problem Summary:
compression=LZ4HCis accepted by the FE, but the tablet metadata conversion omitted the LZ4HC case. It therefore fell through to the LZ4F default and silently changed the requested codec.This PR adds explicit LZ4HC mappings in both the BE tablet metadata conversion and the cloud FE tablet metadata builder, with regression tests for both paths.
Release note
Fix
compression=LZ4HCbeing silently stored as LZ4F in tablet metadata.Check List (For Author)
Test details:
sh run-fe-ut.sh --run "org.apache.doris.cloud.datasource.CloudInternalCatalogTest"(8 tests passed)The changed BE production and test translation units compile successfully. The full BE unit-test binary could not be linked in the local environment because the prescribed
installed-doris-masterpackage is missinglibpaimon_c.a.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)