Skip to content

[fix](compression) Preserve LZ4HC in tablet metadata - #69574

Open
gavinchou wants to merge 2 commits into
apache:masterfrom
gavinchou:gavin-fix-doris-28505-lz4hc-compression
Open

gavinchou wants to merge 2 commits into
apache:masterfrom
gavinchou:gavin-fix-doris-28505-lz4hc-compression

Conversation

@gavinchou

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: DORIS-28505

Problem Summary:

compression=LZ4HC is 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=LZ4HC being silently stored as LZ4F in tablet metadata.

Check List (For Author)

  • Test
    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

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-master package is missing libpaimon_c.a.

  • Behavior changed:

    • No.
    • Yes. LZ4HC table compression now remains LZ4HC in tablet metadata instead of falling back to LZ4F.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@gavinchou
gavinchou requested a review from yiguolei as a code owner October 24, 2026 11:57
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@gavinchou

Copy link
Copy Markdown
Contributor Author

run buildall

@gavinchou

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. Scope and reuse: two explicit switch cases reuse the existing codec; the tests are focused. The FE import needs the inline fix.
  3. 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.
  4. 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.
  5. Configuration: no configuration item or dynamic-update behavior is added or changed.
  6. 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.
  7. Thrift/protobuf IDs: no definition changes; the changed-definition cross-branch ID gate is inapplicable.
  8. 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.
  9. 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.
  10. 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.
  11. Expected results: LZ4HC is the correct new enum expectation; no generated result file is added or modified.
  12. Observability: existing metadata and codec Status/error paths cover this mapping correction; no new operation needs added metrics or logging.
  13. 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.
  14. 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.
  15. FE/BE variable passing: the existing compression_type field already reaches both builders; no new transmitted variable or missing send path.
  16. 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

yiguolei
yiguolei previously approved these changes Oct 24, 2026
@gavinchou

Copy link
Copy Markdown
Contributor Author

run buildall

@gavinchou

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.
  3. 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.
  4. 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.
  5. Configuration: no configuration is added; existing defaults and all other codec branches are preserved.
  6. 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.
  7. Thrift/protobuf evolution: no definitions, field IDs or enum values are changed; a cross-branch ID-change audit is inapplicable.
  8. 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.
  9. 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.
  10. 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.
  11. 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.
  12. Observability: existing tablet-creation identifiers and codec/page failure context cover this correction; no new operation needing separate logging or metrics was identified.
  13. Persistence, transactions and failover: schema serialization/conversion retains compression. No EditLog, transaction-state, replay-order or master-failover protocol change is introduced.
  14. 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.
  15. FE-BE propagation: no new variable is introduced; the existing compression enum reaches both corrected endpoints through their current transport paths.
  16. 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.
  17. 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).

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 28470 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 60a6e13a62ae7b5c863b28c40a8bf8ba1eddc81c, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17622	5075	5044	5044
q2	2105	310	260	260
q3	10240	1364	817	817
q4	4680	463	345	345
q5	7431	794	548	548
q6	185	190	154	154
q7	735	794	595	595
q8	9260	1319	1079	1079
q9	5500	4217	4417	4217
q10	6837	1273	1037	1037
q11	427	240	225	225
q12	640	396	289	289
q13	18073	3615	3023	3023
q14	262	275	247	247
q15	q16	736	740	661	661
q17	1195	753	590	590
q18	7029	6174	6098	6098
q19	1122	942	607	607
q20	382	331	224	224
q21	5260	2152	2318	2152
q22	346	297	258	258
Total cold run time: 100067 ms
Total hot run time: 28470 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	5586	5569	5578	5569
q2	679	562	522	522
q3	5207	5403	4924	4924
q4	1985	2164	1538	1538
q5	5265	5168	5668	5168
q6	263	207	181	181
q7	2302	2124	1968	1968
q8	2976	2698	2720	2698
q9	8167	7966	7912	7912
q10	3743	3691	3283	3283
q11	658	458	432	432
q12	695	679	552	552
q13	3471	3887	3247	3247
q14	323	337	298	298
q15	q16	710	751	651	651
q17	8246	7409	6838	6838
q18	13023	12209	12997	12209
q19	904	784	777	777
q20	2147	2147	1934	1934
q21	5685	4659	4742	4659
q22	494	452	445	445
Total cold run time: 72529 ms
Total hot run time: 65805 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 150788 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 60a6e13a62ae7b5c863b28c40a8bf8ba1eddc81c, data reload: false

query5	4311	624	460	460
query6	419	205	192	192
query7	4810	461	228	228
query8	327	170	146	146
query9	8772	3108	3079	3079
query10	481	307	255	255
query11	5911	3883	3580	3580
query12	145	93	87	87
query13	1253	448	328	328
query14	6167	3900	3668	3668
query14_1	3351	3397	3432	3397
query15	207	198	174	174
query16	936	432	416	416
query17	859	655	535	535
query18	2414	437	314	314
query19	204	171	140	140
query20	88	83	85	83
query21	206	128	106	106
query22	13039	13185	12774	12774
query23	13354	12906	12502	12502
query23_1	12507	12519	12540	12519
query24	7383	827	493	493
query24_1	524	482	479	479
query25	561	425	366	366
query26	1247	261	144	144
query27	2794	455	280	280
query28	4721	1916	1924	1916
query29	1572	596	450	450
query30	303	223	184	184
query31	998	933	821	821
query32	138	93	97	93
query33	525	304	238	238
query34	895	839	481	481
query35	756	747	653	653
query36	809	809	706	706
query37	115	92	78	78
query38	1742	1726	1666	1666
query39	780	782	734	734
query39_1	729	730	739	730
query40	214	112	87	87
query41	66	66	60	60
query42	84	83	85	83
query43	344	365	315	315
query44	1285	679	678	678
query45	174	175	165	165
query46	823	955	562	562
query47	2829	3051	2920	2920
query48	295	297	213	213
query49	576	422	288	288
query50	673	281	235	235
query51	10535	10435	10540	10435
query52	82	85	70	70
query53	187	203	148	148
query54	244	193	195	193
query55	70	65	64	64
query56	230	225	213	213
query57	1677	1627	1562	1562
query58	282	257	255	255
query59	2056	2169	1957	1957
query60	280	239	230	230
query61	149	146	144	144
query62	387	327	288	288
query63	187	151	159	151
query64	2781	939	756	756
query65	3407	3277	3274	3274
query66	1786	412	312	312
query67	20375	20048	19932	19932
query68	3026	1009	646	646
query69	381	307	245	245
query70	835	776	790	776
query71	281	226	204	204
query72	2671	2564	2211	2211
query73	510	519	293	293
query74	4592	4514	4332	4332
query75	2320	2309	1893	1893
query76	2310	1022	566	566
query77	326	379	266	266
query78	9852	9693	9165	9165
query79	1019	859	518	518
query80	547	407	345	345
query81	524	312	271	271
query82	571	132	105	105
query83	298	192	182	182
query84	319	116	98	98
query85	819	423	377	377
query86	326	229	227	227
query87	1986	1969	1814	1814
query88	3593	2660	2642	2642
query89	324	282	240	240
query90	1875	174	160	160
query91	160	155	127	127
query92	103	89	89	89
query93	1001	1007	578	578
query94	445	319	264	264
query95	579	320	370	320
query96	664	482	233	233
query97	2439	2449	2321	2321
query98	157	152	145	145
query99	768	771	643	643
Total cold run time: 233815 ms
Total hot run time: 150788 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.4 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 60a6e13a62ae7b5c863b28c40a8bf8ba1eddc81c, data reload: false

query1	0.01	0.00	0.01
query2	0.09	0.05	0.06
query3	0.26	0.09	0.09
query4	1.61	0.11	0.11
query5	0.24	0.25	0.25
query6	1.18	0.69	0.68
query7	0.04	0.01	0.01
query8	0.05	0.04	0.04
query9	0.40	0.36	0.35
query10	0.62	0.62	0.60
query11	0.20	0.14	0.14
query12	0.18	0.15	0.14
query13	0.48	0.49	0.49
query14	1.03	1.03	1.01
query15	0.62	0.60	0.60
query16	0.35	0.37	0.33
query17	1.13	1.11	1.12
query18	0.23	0.20	0.19
query19	2.12	2.03	1.93
query20	0.02	0.01	0.01
query21	15.48	0.18	0.14
query22	5.00	0.05	0.05
query23	16.28	0.32	0.13
query24	2.89	0.48	0.44
query25	0.10	0.07	0.06
query26	1.17	0.22	0.16
query27	0.06	0.05	0.06
query28	3.45	0.81	0.36
query29	12.49	4.09	3.24
query30	0.28	0.14	0.10
query31	2.77	0.54	0.31
query32	3.22	0.60	0.49
query33	3.27	3.25	3.28
query34	15.33	3.99	3.37
query35	3.32	3.33	3.33
query36	0.56	0.43	0.44
query37	0.10	0.07	0.06
query38	0.05	0.04	0.04
query39	0.04	0.03	0.03
query40	0.17	0.14	0.15
query41	0.09	0.03	0.03
query42	0.04	0.03	0.03
query43	0.05	0.04	0.03
Total cold run time: 97.07 s
Total hot run time: 24.4 s

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 66.58% (31988/48047)
Line Coverage 51.39% (337516/656798)
Region Coverage 46.74% (273042/584189)
Branch Coverage 48.49% (128739/265486)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100% (0/0) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 77.29% (35979/46552)
Line Coverage 62.59% (406683/649761)
Region Coverage 59.04% (343638/582079)
Branch Coverage 59.98% (158862/264837)

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 100.00% (2/2) 🎉
Increment coverage report
Complete coverage report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants