feat(compression): support per-column compression for non-cloud - #66169
feat(compression): support per-column compression for non-cloud#66169zwy991114 wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
1 similar comment
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for seven P1 findings and one P2 finding. No additional user-provided review focus was supplied, so I reviewed the whole PR.
Critical checkpoint conclusions:
- Goal and scope: the PR coherently adds a per-column codec and optional level across SQL, FE catalog/protocol, tablet schema, and both segment writers. The ordinary non-cloud scalar path is largely connected, but cloud/full schema change, compression-only MODIFY, light CCR SQL, complex AGG_STATE storage, codec lifetime/accounting, and test gaps mean the goal is not safely complete.
- Size and focus: the cross-module scope is necessary for this feature and unrelated production changes were not found.
- Concurrency and lifecycle: codec pools are mutex-protected, writer ownership keeps the observer pointer valid, and no new race, lock-order, or deadlock issue was found. The new owned-codec lifecycle does create the retained-workspace multiplier and allocation/free tracker mismatch called out inline.
- Configuration and compatibility: no configuration item is added. Optional thrift/protobuf/footer fields remain readable by compatible readers; old BEs can ignore the write policy during a rolling upgrade and emit the table codec, which is storage-policy drift rather than unreadable data.
- Parallel paths and conditions: legacy, vertical, row-binlog, schema-change, and compaction writers implement small-segment suppression > explicit override > table default when every physical schema node carries the policy. The cloud protobuf omission and complex AGG_STATE child/auxiliary metas are the exceptions. Compression in general Column.equals() also reaches routing/type guards that should not treat it as a logical change.
- Tests and results: none of the changed tests observes requested-level application; the regression write is below the compression-suppression threshold, and the suite lacks the required generated result contract. Per the review-only task contract I did not run local builds/tests; this checkout also lacks .worktree_initialized, thirdparty/installed, and protoc. Live CI currently has compile, FE UT, macOS BE UT, style, license, and secret checks passing; BE UT is ERROR, Cloud UT is FAILURE, several regression/performance checks are pending, and the PR title checker is failing.
- Observability and persistence: SHOW CREATE from the catalog column renders the policy, but reconstructed command SQL omits it. Local edit-log replay retains schema objects; the light CCR binlog executes the incomplete rawSql and loses the clause. Full cloud shadow-tablet protobuf construction separately drops the fields.
- Data writes and transactions: no visible-version, transaction, delete-bitmap, or atomicity mechanism is changed. Segments remain readable because the actual codec is stored in the footer, but affected paths silently write a different policy from the DDL.
- Performance: a private reusable native workspace per levelled column/open segment is an unbounded schema-width multiplier and is not included in segment buffer estimation.
- Other: no security-sensitive behavior, new metric, or dynamic configuration requirement was introduced.
PR metadata also needs cleanup: the title does not match the required bracketed format (for example, [feature](compression) Support per-column compression for non-cloud); Issue Number and Related PR still contain #xxx; and this user-visible feature is marked both Release note: None and Behavior changed: No while documentation is checked without a link.
| } catch (Exception e) { | ||
| throw new AnalysisException(e.getMessage(), e); | ||
| } | ||
| if (compressionType != null && !isOlap) { |
There was a problem hiding this comment.
[P1] Preserve this setting on the cloud tablet-schema path (or reject it in cloud mode). isOlap is also true for cloud OlapTable, but CloudInternalCatalog.createTabletMetaBuilder() serializes columns through ColumnToProtobuf.toPb(), which never writes the new ColumnPB.compression_type/compression_level. A full cloud ADD/MODIFY job therefore builds its shadow tablet without the override and rewrites historical rows with the table codec; later direct loads can receive the thrift fields and use a different codec. Please update that protobuf producer and add cloud coverage, or explicitly gate cloud DDL if cloud support is out of scope.
| && Objects.equals(realDefaultValue, other.realDefaultValue) | ||
| && clusterKeyId == other.clusterKeyId; | ||
| && clusterKeyId == other.clusterKeyId | ||
| && Objects.equals(compressionType, other.compressionType) |
There was a problem hiding this comment.
[P1] Do not let a compression-only delta become a logical/routing change. With these fields in general Column.equals(), SchemaChangeHandler.checkTable() now rejects ALTER ... MODIFY on range/list partition columns and hash-distribution columns because their full equality changed. For ordinary non-VARCHAR scalars, processModifyColumn() also sets typeChanged=true and falls into a full shadow-column rewrite even though the codec is per-segment metadata. Keep compression in no-op detection, but use routing/type-specific comparisons (or explicitly recognize a compression-only delta) for these guards and for light-schema-change classification; add partition/distribution and non-VARCHAR MODIFY tests.
| } | ||
| switch (type) { | ||
| case segment_v2::CompressionTypePB::ZSTD: { | ||
| auto instance = std::make_unique<ZstdBlockCompression>(level); |
There was a problem hiding this comment.
[P1] Avoid a private context pool per levelled column. ScalarColumnWriter owns this instance, and after the first page its ZstdBlockCompression/Lz4HCBlockCompression retains a native context plus reusable buffer until the whole segment clears its column writers. Wide schemas (and multiple open segments) therefore retain one heavyweight workspace per levelled column even though compression is serial; the old singleton pool reused contexts according to actual concurrency, and this memory is not included in the segment-size estimate. Please share/reset pools by codec+level (or release rather than pool per-column contexts) and add a wide-schema tracker test.
| break; | ||
| } | ||
| case segment_v2::CompressionTypePB::LZ4HC: { | ||
| auto instance = std::make_unique<Lz4HCBlockCompression>(level); |
There was a problem hiding this comment.
[P1] Balance native context allocation and destruction under the same tracker for these new short-lived instances. Both _acquire_compression_ctx() paths allocate the LZ4/ZSTD native context (and ZSTD later allocates workspace) under the current load/schema-change tracker, while the context/codec destructors switch to block_compression_mem_tracker() before freeing it. MemTrackerLimiter explicitly requires allocation and free to hit the same tracker; owned per-column codecs now repeat this mismatch at every segment teardown. Put creation/lazy allocation under the compression tracker as well, or keep the entire owned lifetime consistently task-local, and test tracker balance.
| // into ColumnMetaPB, and the reader decompresses (which does not need the level). | ||
| // | ||
| // A tiny data_page_size forces many data pages so the codec is invoked once per | ||
| // page -- this is what guards the LZ4HC "first page uses the default level" |
There was a problem hiding this comment.
[P1] This does not guard the claimed first-page level regression: data compressed at the default LZ4HC/ZSTD level decompresses identically, because the decompressor never needs the level. The test also writes compression_level into the in-memory meta itself, so that assertion is tautological, and the regression suite's three-row write is below the 256 KiB suppression threshold and exercises NO_COMPRESSION. Removing the level constructor/reset code would leave all changed tests green. Please add an observable first/reused-page level oracle and inspect an actual compressed segment footer/page.
| // under the License. | ||
|
|
||
| suite("test_column_compression") { | ||
| def tableName = "test_column_compression_tbl" |
There was a problem hiding this comment.
[P2] Please convert this suite to the required Doris regression-test form before relying on it as feature coverage: hardcode the ordinary single-table name, express determined SHOW/ordered SELECT results with qt/order_qt and generate the matching .out via the runner, and remove the final DROP so failed state remains debuggable. The current direct assertions have no generated result artifact and conflict with the repository test contract.
| } | ||
|
|
||
| /** Called by the parser to attach a COMPRESSION clause. */ | ||
| public void setCompressionSpec(String spec) throws org.apache.doris.common.AnalysisException { |
There was a problem hiding this comment.
[P1] Include the compression clause in reconstructed column SQL. This setter stores the policy, but ColumnDefinition.toSql(String, boolean) stops after COMMENT, so CREATE/ADD/MODIFY reconstruction omits COMPRESSION 'algo[:level]'. Nereids ALTER passes that incomplete SQL into SchemaChangeHandler; the light schema-change binlog/CCR path executes TableAddOrDropColumnsInfo.rawSql, so an ADD loses the policy on the target, while full ALTER_JOB records also expose incomplete SQL metadata. Please render the clause and add CREATE/ADD/MODIFY toSql() plus light-CCR/binlog coverage.
| // the bulk of the bytes live in child/sub-columns that never receive the override, so the | ||
| // requested codec would be silently dropped for that data. Reject it instead. | ||
| if (compressionType != null | ||
| && (type.isArrayType() || type.isMapType() || type.isStructType() |
There was a problem hiding this comment.
[P1] Treat complex serialized AGG_STATE storage like the complex types rejected here. AggStateType fails all four checks, so DDL accepts AGG_STATE<ARRAY_AGG(INT)> COMPRESSION 'zstd:9' (and MAP_AGG). BE dispatches these states to ARRAY/MAP writers: their item/key/value children have no override and use the table codec, while offset/null metas copy the parent codec but omit its level. Both segment writers therefore store one logical state with inconsistent policy. Reject such AGG_STATE types, or propagate codec and level to every physical child/auxiliary meta, and add ARRAY_AGG/MAP_AGG footer coverage.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 29478 ms |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-DS: Total hot run time: 177512 ms |
ClickBench: Total hot run time: 25.77 s |
3 participants
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Doris only supports a single compression codec configured at the table level (via PROPERTIES("compression"=...)), applied uniformly to every column. In real workloads, columns have very different data characteristics — highly redundant text columns (URLs, referers, search phrases) compress far better under a high-level ZSTD/LZ4HC codec, while numeric/low-cardinality columns gain little and pay the CPU cost. There was no way to tune the codec per column.
This PR adds per-column generic compression codec support for non-cloud (OLAP) tables. Users can specify a codec and optional level directly on a column:
Compression ratio test on ClickBench hits:
Loaded the ClickBench hits dataset into two tables with identical schema. One table (baseline) uses the ZSTD level 3 default on every column; the other overrides five heavy text columns with COMPRESSION 'zstd:19'. After forcing full compaction on both, per-column sizes were read from information_schema.column_data_sizes.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)