Uh oh!
There was an error while loading. Please reload this page.
[fix](iceberg)Support LZ4 compression for iceberg/hive Parquet/ORC writers. - #64723
Conversation
hello-stephen
commented
Jun 23, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
hubgeter
commented
Jun 23, 2026
run buildall |
hello-stephen
commented
Jun 23, 2026
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jun 23, 2026
TPC-H: Total hot run time: 29204 ms |
hello-stephen
commented
Jun 23, 2026
TPC-DS: Total hot run time: 172264 ms |
hello-stephen
commented
Jun 23, 2026
ClickBench: Total hot run time: 25.29 s |
hello-stephen
commented
Jun 23, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hubgeter
commented
Jun 23, 2026
/review |
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 2:55 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Gabriel39
commented
Jun 24, 2026
/review |
There was a problem hiding this comment.
Review result: request changes.
Critical checkpoint conclusions:
- Goal and proof: the BE mapping for Iceberg/Hive LZ4 is focused, but Hive DDL propagation is incomplete and the regression test does not prove the required Hadoop-framed Parquet codec.
- Scope: the code changes are small, but the user-facing Hive validation path was not updated with the new writer capability.
- Concurrency and lifecycle: no new concurrency, dependency, or lifecycle-sensitive behavior found in the changed paths.
- Compatibility/config propagation: the thrift enum is appended and Linux compile passed, but Hive Parquet/ORC compression validation still rejects lz4.
- Parallel paths: Iceberg data files, Iceberg delete files, Hive Parquet, and ORC share this feature area; test coverage currently reaches only Iceberg Parquet data-file inserts.
- Test coverage/results: the added test uses hand assertions for deterministic results, lacks generated expected output, does not inspect the Parquet footer codec, misses several changed paths, and cleans up at the end against the regression-test standard.
- CI observed: Linux compile, FE UT, BE UT, P0 regression, external regression, checkstyle, formatter, and license checks are passing. The macOS BE UT failure is environment setup (JDK 25 where JDK 17 is required), not tied to this diff; coverage checks are failing externally.
- User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite found no optimizer/join/aggregate issues. tests-session-config candidates TSC-001 and TSC-002 were merged into MAIN-002, TSC-003 became MAIN-003, and convergence candidate TSC-004 became MAIN-004. Convergence round 2 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the current ledger/comment set.
| parquet_compression_type = TParquetCompressionType::ZSTD; | ||
| break; | ||
| } | ||
| case TFileCompressType::LZ4BLOCK: { |
There was a problem hiding this comment.
This BE path now accepts Hive Parquet LZ4BLOCK, but the Doris Hive DDL path still rejects the same user-facing setting. HiveUtil.setCompressType() only allows plain/snappy/zstd for Parquet and plain/zlib/snappy/zstd for ORC, so CREATE TABLE ... ENGINE=hive PROPERTIES ('file_format'='parquet', 'compression'='lz4') still fails before it can reach this writer. Please add lz4 to the Hive Parquet/ORC validation path and cover it in the Hive DDL/write tests so the advertised Hive LZ4 writer support is reachable from Doris-created Hive tables.
| def expected = [["doris0", 0L], ["doris1", 1L], ["doris2", 2L]] | ||
| // Codecs Doris is expected to support for Parquet writes. | ||
| for (String codec : ["lz4", "snappy", "zstd", "uncompressed"]) { |
There was a problem hiding this comment.
This loop only covers Iceberg Parquet data-file inserts. The PR also changes ORC LZ4 handling (VOrcTransformer), Hive Parquet LZ4 handling (VHivePartitionWriter), and Iceberg delete-file compression (VIcebergDeleteFileWriter), but none of those paths are exercised here or by the existing LZ4 suites I found. Please add focused coverage for the other changed paths, at least Hive Parquet LZ4 and Iceberg/Hive ORC LZ4 writes; if delete files inherit the table compression, add a delete/update scenario that drives the LZ4 VIcebergDeleteFileWriter path too.
| sql """ INSERT INTO ${tbl} VALUES ('doris0', 0), ('doris1', 1), ('doris2', 2) """ | ||
| def rows = sql """ SELECT a, b FROM ${tbl} ORDER BY b """ |
There was a problem hiding this comment.
This only round-trips the rows through Doris with Groovy assertEquals, so it misses two important checks. First, deterministic regression results should be captured with order_qt/generated .out rather than hand assertions. Second, this PR specifically needs to prove the file footer says Parquet LZ4 (Hadoop-framed), not LZ4_RAW; Doris' reader handles both, so this query would still pass if the writer accidentally used TParquetCompressionType::LZ4. Please add a generated-output check that inspects the written file metadata, for example through the existing parquet_meta metadata mode compression column, and expects LZ4 for the LZ4 table.
| exception "Unsupported compress type GZ with parquet" | ||
| } | ||
| sql """ drop database if exists ${db} force """ |
There was a problem hiding this comment.
Please remove the final database/catalog cleanup. The suite already drops the catalog/database before creating them, and the regression-test standard asks new tests to clean before use rather than after completion so the created objects remain available for debugging a failed or suspicious run.
hubgeter
commented
Jun 24, 2026
run buildall |
hubgeter
commented
Jun 24, 2026
/review |
There was a problem hiding this comment.
I found one remaining test coverage gap after the current updates.
Critical checkpoint conclusions:
- Goal/test proof: the BE/FE mapping for Hive and Iceberg LZ4 is now reachable, but the Hive Parquet path still lacks proof that it writes Hadoop-framed Parquet LZ4 rather than raw LZ4.
- Scope/focus: the implementation changes are otherwise small and focused on compression validation/mapping plus regression coverage.
- Concurrency/lifecycle/config: no new concurrency, lifecycle ownership, dynamic config, or persistence path is introduced by this PR scope.
- FE-BE compatibility: the existing
TFileCompressType.LZ4BLOCKsink path is used for FE-to-BE communication; the new Parquet compression enum is consumed inside BE writer option construction. - Parallel paths: Iceberg data/delete and Hive ORC/Parquet paths were checked. The remaining gap is Hive Parquet footer verification.
- Tests: Iceberg now has generated
order_qtoutput and footer metadata checks; Hive LZ4 currently has only row readback.
Subagent conclusions: tests-session-config proposed TSC-001, which I verified and submitted as the inline comment below. optimizer-rewrite found no optimizer/rewrite candidate. In convergence round 1, both subagents replied NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set after MAIN-001 was accepted.
User focus: no additional user-provided review focus was supplied.
| if (compression.equals("lz4")) { | ||
| sql """ INSERT INTO tbl_${file_format}_${compression} VALUES ('doris_lz4') """ | ||
| def q_lz4 = "order_qt_hive_${file_format}_${compression}_write" | ||
| "${q_lz4}" """ SELECT * FROM tbl_${file_format}_${compression} ORDER BY col """ |
There was a problem hiding this comment.
The Hive Parquet LZ4 branch still only proves that Doris can read the row back. That does not protect the compatibility fix here, because VHivePartitionWriter now deliberately maps LZ4BLOCK to TParquetCompressionType::LZ4_HADOOP, while the Parquet helper still has the separate raw TParquetCompressionType::LZ4 path, and Doris can read both variants. Please add a Hive Parquet metadata assertion similar to the Iceberg one, using the generated Hive file path with parquet_meta(..., mode = "parquet_metadata") and expecting footer compression = LZ4, so a regression back to LZ4_RAW is caught.
hubgeter
commented
Jun 24, 2026
run buildall |
hello-stephen
commented
Jun 24, 2026
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jun 24, 2026
TPC-H: Total hot run time: 29520 ms |
hello-stephen
commented
Jun 24, 2026
TPC-H: Total hot run time: 29335 ms |
hello-stephen
commented
Jun 24, 2026
TPC-DS: Total hot run time: 171256 ms |
hello-stephen
commented
Jun 24, 2026
ClickBench: Total hot run time: 25.32 s |
hello-stephen
commented
Jun 24, 2026
TPC-H: Total hot run time: 29115 ms |
hello-stephen
commented
Jun 24, 2026
TPC-DS: Total hot run time: 172884 ms |
hello-stephen
commented
Jun 24, 2026
ClickBench: Total hot run time: 25.93 s |
hello-stephen
commented
Jun 24, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jun 24, 2026
FE Regression Coverage ReportIncrement line coverage |
Gabriel39
commented
Jun 25, 2026
/review |
There was a problem hiding this comment.
I found one remaining correctness gap in the LZ4 compatibility fix.
Critical checkpoint conclusions:
- Goal/test proof: Hive/Iceberg LZ4 paths are now reachable and have footer metadata tests, but large Parquet pages can still be unreadable by JVM engines.
- Scope/focus: implementation is focused, but the compatibility claim needs either an Arrow codec fix, page-size constraints, or narrower wording.
- Concurrency/lifecycle/config: no new concurrency, lifecycle ownership, dynamic config, or persistence paths.
- FE-BE compatibility: FE passes the existing
TFileCompressType.LZ4BLOCK; the newTParquetCompressionType.LZ4_HADOOPvalue is consumed inside BE writer option construction. - Parallel paths: Hive/Iceberg Parquet/ORC and delete paths were reviewed; ORC mapping is consistent with ORC 1.9 LZ4 support.
- Tests: generated-output checks now cover small-file footer metadata and reachability, but not the large-page JVM compatibility case.
Subagent conclusions: optimizer-rewrite appended OPT-NONE; tests-session-config proposed TSC-001 and TSC-002. TSC-001 was dismissed because it came from unrelated files outside PR 64723's actual diff; TSC-002 became inline comment MAIN-001. Convergence round 1 ended with both subagents reporting NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set.
User focus: no additional user-provided review focus was supplied.
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
Jun 25, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jun 25, 2026
FE Regression Coverage ReportIncrement line coverage |
hubgeter
commented
Jun 25, 2026
/review |
There was a problem hiding this comment.
Reviewed the current PR 64723 scope against the changed-file list, existing review threads, and the Doris review checklist.
Critical checkpoints: the implementation now makes Hive/Iceberg LZ4 compression reachable through FE validation and external sink compression mapping, and the BE Parquet paths use the new Hadoop-framed LZ4_HADOOP option with capped page targets. No optimizer/rewrite, concurrency, lifecycle, dynamic config, persistence, or FE-BE mixed-version protocol issue was found in the scoped changes. Parallel writer paths were checked across Hive Parquet, Hive ORC, Iceberg data files, and Iceberg delete files. The remaining issue is test coverage: the ORC LZ4 paths still only prove readable ORC output, not that the written ORC codec is actually LZ4.
User focus: no additional user-provided focus was supplied.
Tests were not run in this checkout because .worktree_initialized and thirdparty/installed are absent, and the available setup hook requires copying dependencies from an external ROOT_WORKSPACE_PATH, which this review prompt forbids. I did run git diff --check restricted to the 11 PR changed files, and it passed.
Subagent conclusions: optimizer-rewrite reported no optimizer/rewrite candidate. tests-session-config proposed the ORC-codec assertion gap, which I accepted as the inline comment below. Final convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same current ledger/comment set.
Uh oh!
There was an error while loading. Please reload this page.
hubgeter
commented
Jun 25, 2026
run buildall |
hello-stephen
commented
Jun 25, 2026
TPC-H: Total hot run time: 29242 ms |
hello-stephen
commented
Jun 25, 2026
TPC-DS: Total hot run time: 171033 ms |
hello-stephen
commented
Jun 25, 2026
ClickBench: Total hot run time: 25.27 s |
hello-stephen
commented
Jun 25, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jun 25, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jun 25, 2026
FE Regression Coverage ReportIncrement line coverage |
PR approved by at least one committer and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
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)