github-actions[bot] commented on code in PR #66169:
URL: https://github.com/apache/doris/pull/66169#discussion_r3666194566
##########
fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java:
##########
@@ -918,7 +951,9 @@ public boolean equals(Object obj) {
&& visible == other.visible
&& Objects.equals(children, other.children)
&& Objects.equals(realDefaultValue, other.realDefaultValue)
- && clusterKeyId == other.clusterKeyId;
+ && clusterKeyId == other.clusterKeyId
+ && Objects.equals(compressionType, other.compressionType)
Review 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.
##########
be/src/util/block_compression.cpp:
##########
@@ -1615,6 +1623,36 @@ Status
get_block_compression_codec(segment_v2::CompressionTypePB type,
return Status::OK();
}
+Status get_block_compression_codec(segment_v2::CompressionTypePB type, int
level,
+ std::unique_ptr<BlockCompressionCodec>*
codec_owned,
+ BlockCompressionCodec** codec) {
+ codec_owned->reset();
+ // level <= 0 means "use codec default" -> fall back to the stateless
singleton path.
+ if (level <= 0) {
+ return get_block_compression_codec(type, codec);
+ }
+ switch (type) {
+ case segment_v2::CompressionTypePB::ZSTD: {
+ auto instance = std::make_unique<ZstdBlockCompression>(level);
+ RETURN_IF_ERROR(instance->init());
+ *codec = instance.get();
+ *codec_owned = std::move(instance);
+ break;
+ }
+ case segment_v2::CompressionTypePB::LZ4HC: {
+ auto instance = std::make_unique<Lz4HCBlockCompression>(level);
Review 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.
##########
be/src/util/block_compression.cpp:
##########
@@ -1615,6 +1623,36 @@ Status
get_block_compression_codec(segment_v2::CompressionTypePB type,
return Status::OK();
}
+Status get_block_compression_codec(segment_v2::CompressionTypePB type, int
level,
+ std::unique_ptr<BlockCompressionCodec>*
codec_owned,
+ BlockCompressionCodec** codec) {
+ codec_owned->reset();
+ // level <= 0 means "use codec default" -> fall back to the stateless
singleton path.
+ if (level <= 0) {
+ return get_block_compression_codec(type, codec);
+ }
+ switch (type) {
+ case segment_v2::CompressionTypePB::ZSTD: {
+ auto instance = std::make_unique<ZstdBlockCompression>(level);
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinition.java:
##########
@@ -171,6 +176,71 @@ public ColumnDefinition(String name, DataType type,
boolean isNullable, String c
this(name, type, false, null, isNullable, Optional.empty(), comment);
}
+ /** Parsed per-column compression spec. level == -1 means "codec default".
*/
+ public static class CompressionSpec {
+ public final TCompressionType type;
+ public final int level;
+
+ public CompressionSpec(TCompressionType type, int level) {
+ this.type = type;
+ this.level = level;
+ }
+ }
+
+ /** Parse and validate a per-column COMPRESSION 'algo[:level]' spec. */
+ public static CompressionSpec parseCompressionSpec(String spec) throws
org.apache.doris.common.AnalysisException {
+ if (spec == null || spec.trim().isEmpty()) {
+ throw new org.apache.doris.common.AnalysisException("empty
compression spec");
+ }
+ String[] parts = spec.trim().split(":", 2);
+ String algo = parts[0].trim();
+ TCompressionType type = PropertyAnalyzer.stringToCompressionType(algo);
+ int level = -1;
+ if (parts.length == 2) {
+ String levelStr = parts[1].trim();
+ int parsed;
+ try {
+ parsed = Integer.parseInt(levelStr);
+ } catch (NumberFormatException e) {
+ throw new org.apache.doris.common.AnalysisException("invalid
compression level: " + levelStr);
+ }
+ switch (type) {
+ case ZSTD:
+ if (parsed < 1 || parsed > 22) {
+ throw new org.apache.doris.common.AnalysisException(
+ "ZSTD compression level must be in [1, 22],
got " + parsed);
+ }
+ break;
+ case LZ4HC:
+ if (parsed < 1 || parsed > 12) {
+ throw new org.apache.doris.common.AnalysisException(
+ "LZ4HC compression level must be in [1, 12],
got " + parsed);
+ }
+ break;
+ default:
+ throw new org.apache.doris.common.AnalysisException(
+ "compression level is only supported for ZSTD and
LZ4HC, not " + algo);
+ }
+ level = parsed;
+ }
+ return new CompressionSpec(type, level);
+ }
+
+ /** Called by the parser to attach a COMPRESSION clause. */
+ public void setCompressionSpec(String spec) throws
org.apache.doris.common.AnalysisException {
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinition.java:
##########
@@ -381,6 +451,20 @@ private void validateInternal(boolean isOlap, Set<String>
keysSet, Set<String> c
} catch (Exception e) {
throw new AnalysisException(e.getMessage(), e);
}
+ if (compressionType != null && !isOlap) {
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinition.java:
##########
@@ -381,6 +451,20 @@ private void validateInternal(boolean isOlap, Set<String>
keysSet, Set<String> c
} catch (Exception e) {
throw new AnalysisException(e.getMessage(), e);
}
+ if (compressionType != null && !isOlap) {
+ throw new AnalysisException(
+ "COMPRESSION is only supported for OLAP table columns,
column: " + name);
+ }
+ // A per-column codec only stamps the top-level column meta. For
ARRAY/MAP/STRUCT/VARIANT
+ // 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()
Review 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/test/storage/segment/column_compression_roundtrip_test.cpp:
##########
@@ -0,0 +1,196 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gen_cpp/olap_file.pb.h>
+#include <gen_cpp/segment_v2.pb.h>
+#include <gtest/gtest.h>
+
+#include "common/config.h"
+#include "core/assert_cast.h"
+#include "core/column/column_vector.h"
+#include "io/fs/file_reader.h"
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "storage/olap_common.h"
+#include "storage/segment/column_reader.h"
+#include "storage/segment/column_writer.h"
+#include "storage/tablet/tablet_schema.h"
+
+namespace doris::segment_v2 {
+
+static const std::string TEST_DIR =
"./ut_dir/column_compression_roundtrip_test";
+
+class ColumnCompressionRoundtripTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+ _old_disable_storage_page_cache = config::disable_storage_page_cache;
+ config::disable_storage_page_cache = true;
+ auto st = io::global_local_filesystem()->delete_directory(TEST_DIR);
+ ASSERT_TRUE(st.ok()) << st.to_string();
+ st = io::global_local_filesystem()->create_directory(TEST_DIR);
+ ASSERT_TRUE(st.ok()) << st.to_string();
+ }
+
+ void TearDown() override {
+
EXPECT_TRUE(io::global_local_filesystem()->delete_directory(TEST_DIR).ok());
+ config::disable_storage_page_cache = _old_disable_storage_page_cache;
+ }
+
+private:
+ bool _old_disable_storage_page_cache = false;
+};
+
+// Write `num_rows` INT values through a segment column configured with the
given
+// compression codec + level, reopen the file, read every value back, and
assert
+// the round-trip is lossless. This exercises the full per-column compression
+// plumbing: the writer picks a level-aware codec, the codec/level is persisted
+// 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"
Review 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.
##########
regression-test/suites/ddl_p0/test_column_compression.groovy:
##########
@@ -0,0 +1,113 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_column_compression") {
+ def tableName = "test_column_compression_tbl"
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
