You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "arunkumarucet (via GitHub)" <gi...@apache.org> on 2026/10/08 15:41:56 UTC
[PR] Make the default forward index compression codec configurable [pinot]
arunkumarucet opened a new pull request, #19797:
URL: https://github.com/apache/pinot/pull/19797
## Problem
Forward index defaults are configurable cluster-wide through `pinot.forward.index.default.*` —
raw index writer version, target max chunk size, target docs per chunk — but the **compression
codec is not**. `ForwardIndexType#getDefaultCompressionType` is hardcoded:
```java
if (fieldType == FieldSpec.FieldType.METRIC) {
return ChunkCompressionType.PASS_THROUGH;
} else {
return ChunkCompressionType.LZ4;
}
```
So an operator who wants a different codec across a cluster has to name it in every table config,
column by column. That is not just verbose — it changes behaviour, because a codec named in a table
config is treated as explicit, and `ForwardIndexHandler#shouldChangeRawCompressionType` will then
rewrite forward indexes on reload. A default carries no such cost, by design:
```java
// Note that default compression type (PASS_THROUGH for metric and LZ4 for dimension) is not considered if the
// compressionType is not explicitly provided in tableConfig. This is to avoid incorrectly rewriting all the
// forward indexes during segmentReload when the default compressionType changes.
```
## Change
Adds `pinot.forward.index.default.compression.codec`, applied at startup in
`ServiceStartableUtils#initForwardIndexConfig` next to the existing forward index defaults, backed by
a settable default on `ForwardIndexConfig` like its neighbours.
- **Metrics keep `PASS_THROUGH`.** They are usually small fixed-width values where compression costs
more than it saves, so a cluster-wide default should not silently start compressing them. The new
key replaces the non-metric default only.
- **Table configs still win.** A column naming a codec is unaffected.
- **Existing segments are untouched.** Because this stays a default rather than a table config value,
a reload will not rewrite forward indexes when it changes — the behaviour the comment above
describes.
- **An unparseable value is logged and ignored** rather than failing startup, matching how the
neighbouring forward index defaults behave.
## Testing
`ForwardIndexDefaultCompressionTypeTest` covers the LZ4 default for non-metric fields, the
`PASS_THROUGH` default for metrics, that a configured default applies to DIMENSION / DATE_TIME /
COMPLEX, and that it deliberately does **not** apply to METRIC.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4229544963
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/ServiceStartableUtilsTest.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.common.utils;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Covers parsing of the `pinot.forward.index.default.compression.codec` cluster config.
+///
+/// The value names a [org.apache.pinot.spi.config.table.FieldConfig.CompressionCodec], the same
+/// vocabulary a table config uses, and anything that cannot serve as a raw forward index chunk codec is
+/// ignored rather than applied: it would otherwise be injected after table-config validation has run and
+/// fail at segment build time, across every table at once.
+///
+/// Not thread-safe: every method mutates the process-global [ForwardIndexConfig] default, so this class
+/// must not run in parallel with other tests, or with anything that builds segments.
+public class ServiceStartableUtilsTest {
+
+ @BeforeMethod
+ public void setUp() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @AfterMethod(alwaysRun = true)
+ public void tearDown() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @DataProvider
+ public Object[][] applicableCodecs() {
+ return new Object[][]{
+ {"ZSTANDARD", ChunkCompressionType.ZSTANDARD},
+ {"zstandard", ChunkCompressionType.ZSTANDARD},
+ {" ZSTANDARD ", ChunkCompressionType.ZSTANDARD},
+ {"SNAPPY", ChunkCompressionType.SNAPPY},
+ {"GZIP", ChunkCompressionType.GZIP},
+ {"PASS_THROUGH", ChunkCompressionType.PASS_THROUGH},
+ {"LZ4", ChunkCompressionType.LZ4}
+ };
+ }
+
+ @Test(dataProvider = "applicableCodecs")
+ public void testApplicableCodecIsApplied(String configured, ChunkCompressionType expected) {
+ // Seed something other than the value under test, so a row whose expectation happens to equal the
+ // starting default -- LZ4 -- still fails if the codec is never applied.
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.GZIP);
+ if (expected == ChunkCompressionType.GZIP) {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.SNAPPY);
+ }
+
+ ServiceStartableUtils.setDefaultCompressionCodec(configured);
+
+ assertEquals(ForwardIndexConfig.getDefaultCompressionType(), expected);
+ }
+
+ /// `DELTA` and `DELTADELTA` matter most here: they are rejected by table-config validation on every
+ /// column, and their compressors throw on any chunk whose size is not a multiple of 4 bytes, so every
+ /// raw var-width column in the cluster would fail to build if one slipped through as a default.
+ @DataProvider
+ public Object[][] codecsNotApplicableToRawIndexes() {
+ return new Object[][]{
+ {"DELTA"}, {"DELTADELTA"}, {"MV_ENTRY_DICT"}, {"CLP"}, {"CLPV2"}, {"CLPV2_ZSTD"}, {"CLPV2_LZ4"}
+ };
+ }
+
+ @Test(dataProvider = "codecsNotApplicableToRawIndexes")
Review Comment:
**[severity: low] The rejection-path tests keep the LZ4 baseline, so "rejected" is indistinguishable from "wrongly applied as LZ4".**
The applied-path test was given a non-LZ4 seed exactly for this reason, but these rejection tests (and `testUnrecognisedValueKeepsCurrentDefault`) still start from LZ4: a regression that wrongly *applies* LZ4 for a rejected codec — or maps one to LZ4 — passes unnoticed. Seeding a non-LZ4 value here too (as the applied test does) makes the assertion actually prove the value was kept.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
Review Comment:
**[severity: low] Nothing tests that the config key reaches the setter.**
`initForwardIndexConfig` is private and the tests only exercise the `@VisibleForTesting` `setDefaultCompressionCodec`; no test references `CONFIG_OF_DEFAULT_COMPRESSION_CODEC` through a `PinotConfiguration`, and nothing verifies a built segment's raw forward index actually carries the configured codec (`ForwardIndexDefaultCompressionTypeTest` stops at `ForwardIndexType.getDefaultCompressionType`). A refactor that drops the lookup or reads the wrong constant would pass every test while the knob silently does nothing. `initFieldSpecConfig` is public and exercised through a `PinotConfiguration` elsewhere — the same wiring-level test would close the gap here.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexDefaultCompressionTypeTest.java:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.segment.local.segment.index.forward;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.apache.pinot.spi.data.FieldSpec.FieldType;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Tests that the configurable default compression type is honoured for raw columns whose table config
+/// does not name a codec, and that metrics keep their uncompressed default regardless.
+///
+/// Not thread-safe: every method mutates the process-global [ForwardIndexConfig] default, so this class
+/// must not run in parallel with other tests, or with anything that builds segments.
+public class ForwardIndexDefaultCompressionTypeTest {
Review Comment:
**[severity: low] Consider folding these cases into the existing `ForwardIndexTypeTest`.**
`ForwardIndexTypeTest` in the same package already covers `ForwardIndexType`, and the only distinct lifecycle here is the one-line static reset — which fits as `@BeforeMethod`/`@AfterMethod` on the existing class (repo review guidance prefers extending the production class's existing test class unless setup or scope is materially distinct). Two of the four tests also cover pre-existing behavior (LZ4/PASS_THROUGH defaults) rather than this PR's change.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227363684
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -46,6 +46,7 @@ public class ForwardIndexConfig extends IndexConfig {
private static String _defaultTargetMaxChunkSize = "1MB";
private static int _defaultTargetMaxChunkSizeBytes = 1024 * 1024;
private static int _defaultTargetDocsPerChunk = 1000;
+ private static ChunkCompressionType _defaultCompressionType = ChunkCompressionType.LZ4;
Review Comment:
Done — added. Agreed on the asymmetry with the siblings: a stale read of a chunk-size tweak is a performance wobble, a stale read here changes what lands on disk. Noted in the field's javadoc why it differs from its neighbours.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -76,6 +77,16 @@ public static void setDefaultTargetDocsPerChunk(int defaultTargetDocsPerChunk) {
_defaultTargetDocsPerChunk = defaultTargetDocsPerChunk;
}
+ /// The chunk compression type applied to a raw column whose table config does not name one, except
Review Comment:
Good catch, and the doc was the thing that was wrong, not the method. Reworded: the getter now says it returns the raw value, that the metric exemption is applied by the caller in `ForwardIndexType`, and that a caller resolving a default for a specific column must go through that rather than reading the static directly.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {}, expected one of: {}, keeping: {}",
+ defaultCompressionCodec, rawIndexCompressionCodecs(), ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ if (!codec.isApplicableToRawIndex()) {
+ LOGGER.warn("Forward index default compression codec: {} is not applicable to raw forward indexes, expected "
+ + "one of: {}, keeping: {}", codec, rawIndexCompressionCodecs(),
+ ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ ChunkCompressionType compressionType;
+ try {
+ compressionType = ChunkCompressionType.valueOf(codec.name());
Review Comment:
Agreed, removed. After `isApplicableToRawIndex()` the codec is one of PASS_THROUGH/SNAPPY/ZSTANDARD/LZ4/GZIP and all five have same-named `ChunkCompressionType` constants, so the catch was unreachable and untestable. Now a bare `valueOf` with a comment saying it should fail loudly if that ever stops holding — your point about the canonical switch having a non-identity case (CLP* -> PASS_THROUGH) is exactly why silent rejection would be the wrong failure mode.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexDefaultCompressionTypeTest.java:
##########
@@ -0,0 +1,67 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.segment.local.segment.index.forward;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.apache.pinot.spi.data.FieldSpec.FieldType;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Tests that the configurable default compression type is honoured for raw columns whose table config
+/// does not name a codec, and that metrics keep their uncompressed default regardless.
+public class ForwardIndexDefaultCompressionTypeTest {
+
+ @AfterMethod(alwaysRun = true)
+ public void tearDown() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @Test
+ public void testDefaultIsLZ4ForNonMetric() {
Review Comment:
Fixed — added `@BeforeMethod` mirroring `ServiceStartableUtilsTest`. You're right that it was passing only because of `reuseForks=false`; the comment now says the reset is load-bearing for `testDefaultIsLZ4ForNonMetric`, not just cleanup.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/ServiceStartableUtilsTest.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.common.utils;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Covers parsing of the `pinot.forward.index.default.compression.codec` cluster config.
+///
+/// The value names a [org.apache.pinot.spi.config.table.FieldConfig.CompressionCodec], the same
+/// vocabulary a table config uses, and anything that cannot serve as a raw forward index chunk codec is
+/// ignored rather than applied: it would otherwise be injected after table-config validation has run and
+/// fail at segment build time, across every table at once.
+public class ServiceStartableUtilsTest {
+
+ @BeforeMethod
+ public void setUp() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @AfterMethod(alwaysRun = true)
+ public void tearDown() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @DataProvider
+ public Object[][] applicableCodecs() {
+ return new Object[][]{
+ {"ZSTANDARD", ChunkCompressionType.ZSTANDARD},
+ {"zstandard", ChunkCompressionType.ZSTANDARD},
+ {" ZSTANDARD ", ChunkCompressionType.ZSTANDARD},
+ {"SNAPPY", ChunkCompressionType.SNAPPY},
+ {"GZIP", ChunkCompressionType.GZIP},
+ {"PASS_THROUGH", ChunkCompressionType.PASS_THROUGH},
+ {"LZ4", ChunkCompressionType.LZ4}
Review Comment:
Fixed. The test now seeds a different codec before the call (GZIP, or SNAPPY for the GZIP row) so no row can pass on a no-op, including the LZ4 one.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/ServiceStartableUtilsTest.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.common.utils;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Covers parsing of the `pinot.forward.index.default.compression.codec` cluster config.
+///
+/// The value names a [org.apache.pinot.spi.config.table.FieldConfig.CompressionCodec], the same
+/// vocabulary a table config uses, and anything that cannot serve as a raw forward index chunk codec is
+/// ignored rather than applied: it would otherwise be injected after table-config validation has run and
+/// fail at segment build time, across every table at once.
+public class ServiceStartableUtilsTest {
Review Comment:
Added to both test classes: a line stating they mutate the process-global `ForwardIndexConfig` default and must not run in parallel with other tests or with anything that builds segments.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19797:
URL: https://github.com/apache/pinot/pull/19797#issuecomment-6119334073
Documentation follow-up merged: https://github.com/pinot-contrib/pinot-docs/pull/1098. Covers the default codec setting, supported values and precedence, service restart scope, external ingestion limits, and reload behavior.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4229537268
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved. The warning names the codecs that are
+ /// accepted, so a rejected value is self-correcting from the log.
+ ///
+ /// Scope, which this being a *cluster* config does not make as wide as it sounds:
+ ///
+ /// - It is read once per JVM at startup, so after changing it a rolling restart leaves restarted
+ /// instances writing the new codec while the rest keep the old one until they restart too. Each
+ /// segment records the codec it was written with, so this mixes formats without breaking reads.
+ /// - It reaches only components that call [#applyClusterConfig]: the controller, broker, server and
+ /// minion. Segments built outside the cluster -- standalone, Spark or Hadoop batch ingestion, and
+ /// `pinot-admin CreateSegment` -- keep the compiled-in default, and because a reload ignores
+ /// defaults, nothing later reconciles the two.
+ /// - It is a process-global default, so in a single-JVM multi-role deployment the last role to start
+ /// wins for the whole process.
+ ///
+ /// These all hold for the three forward index defaults above as well; they are spelled out here
+ /// because this is the first of them an operator is routinely expected to set.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {}, expected one of: {}, keeping: {}",
+ defaultCompressionCodec, rawIndexCompressionCodecs(), ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ if (!codec.isApplicableToRawIndex()) {
Review Comment:
**[severity: high] Allowing GZIP as a cluster-wide default exposes every raw column to the non-thread-safe `GzipDecompressor`.**
`GzipDecompressor` wraps a single `java.util.zip.Inflater` and runs `reset()` / `setInput()` / `inflate()` as an unsynchronized sequence. `BaseChunkForwardIndexReader` stores one decompressor per reader (field `_chunkDecompressor`, used directly in `decompress()`), and `VarByteChunkForwardIndexReaderV4` shares one instance across all reader contexts — while the reader contract (`ForwardIndexReaderContext`) requires readers to be stateless because they are accessed by multiple threads. The other codecs (LZ4, ZSTD, Snappy, pass-through) use stateless decompressors, so GZIP is the only affected one.
Today this race is reachable only for columns that explicitly configure GZIP. With `pinot.forward.index.default.compression.codec=GZIP`, every raw non-metric column without an explicit codec is built with GZIP, and two concurrent queries on the same segment column can interleave (A: reset+setInput(chunkA); B: reset+setInput(chunkB); A: inflate -> gets chunkB's bytes), silently returning wrong values or throwing `DataFormatException`/OOB.
Consider either excluding GZIP from the accepted defaults until the decompressor is thread-safe, or fixing `GzipDecompressor` (ThreadLocal `Inflater`, or a fresh decompressor per reader context) in/before this PR.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227370142
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
Review Comment:
Documented rather than fixed, and I want to be explicit that it is a real limitation.
The method doc now spells out all three scope caveats together: the value is read once per JVM at startup, it reaches only components that call `applyClusterConfig`, and it is a process-global default so in a single-JVM multi-role deployment the last role to start wins for the whole process. It also says these hold for the three sibling defaults, and that they are spelled out here because this is the first of them an operator is routinely expected to set.
Fixing the single-JVM case properly means making the default per-role rather than per-process, which is a change to all four defaults and to `ForwardIndexConfig`'s contract — happy to do it as a follow-up if you think it is worth it, but it felt wrong to smuggle into this PR.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -3024,6 +3024,8 @@ public static class ForwardIndexConfigs {
"pinot.forward.index.default.target.max.chunk.size";
public static final String CONFIG_OF_DEFAULT_TARGET_DOCS_PER_CHUNK =
"pinot.forward.index.default.target.docs.per.chunk";
+ public static final String CONFIG_OF_DEFAULT_COMPRESSION_CODEC =
+ "pinot.forward.index.default.compression.codec";
Review Comment:
Agreed the doc over-promised. `ForwardIndexType.getDefaultCompressionType`'s javadoc now says the config is applied at startup by components that read cluster config, and that segments built outside the cluster — standalone, Spark or Hadoop batch ingestion — keep LZ4. The same three caveats, including this one, are listed on the parsing method in `ServiceStartableUtils`.
I left the behaviour alone: making `SegmentGenerationTaskRunner` and `pinot-admin CreateSegment` honour a cluster config they never read would mean giving them a ZK connection, which is a much larger change and would be odd for the sibling defaults to not share.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -335,11 +335,20 @@ private ForwardIndexConfig createConfigFromFieldConfig(FieldConfig fieldConfig,
return builder.build();
}
+ /// The compression type used for a raw column whose table config does not name one.
+ ///
+ /// Metrics stay uncompressed: they are usually small fixed-width values where compression costs more
+ /// than it saves. Everything else uses [ForwardIndexConfig#getDefaultCompressionType()], which is LZ4
+ /// unless `pinot.forward.index.default.compression.codec` says otherwise.
+ ///
+ /// This is a default, not a table config value, so changing it does not rewrite existing segments:
+ /// `ForwardIndexHandler` deliberately ignores it when deciding whether a reload must rewrite a forward
+ /// index.
public static ChunkCompressionType getDefaultCompressionType(FieldSpec.FieldType fieldType) {
if (fieldType == FieldSpec.FieldType.METRIC) {
return ChunkCompressionType.PASS_THROUGH;
} else {
- return ChunkCompressionType.LZ4;
+ return ForwardIndexConfig.getDefaultCompressionType();
Review Comment:
Verified and documented. `noDictionaryConfig` is consumed by `keySet()` only — `ForwardIndexType` L239-241 and `DictionaryIndexType` L127-128 — so the per-column codec value has always been discarded, and those columns fall through to the null-codec path in `ForwardIndexCreatorFactory`.
You are right that this PR changes the substitute from a deterministic LZ4 to a cluster-mutable value, so I have said so in the method javadoc: it now notes that the legacy config's codec value is discarded, that only its key set is read, and that such a column used to land on a hardcoded LZ4 and now follows the configured default.
I did not make it honour the legacy value here. That would be a behaviour fix to a config whose value has been ignored for a long time, and it belongs in its own change where it can be considered on its merits rather than as a side effect of adding a default.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -335,11 +335,20 @@ private ForwardIndexConfig createConfigFromFieldConfig(FieldConfig fieldConfig,
return builder.build();
}
+ /// The compression type used for a raw column whose table config does not name one.
+ ///
+ /// Metrics stay uncompressed: they are usually small fixed-width values where compression costs more
+ /// than it saves. Everything else uses [ForwardIndexConfig#getDefaultCompressionType()], which is LZ4
Review Comment:
Good catch on the rationale, which is the part that actually broke. The comment said "LZ4 preserves the dense child's current on-disk layout", which was only true while LZ4 *was* the default; it now reads that LZ4 is pinned there regardless of the cluster-wide default compression codec.
I kept both sites pinned to LZ4 rather than routing them through the default. The split re-encodes an existing dense child, so the codec choice there is about matching what is already on disk, not about what new columns should use — routing it through a mutable default would make the re-encode depend on cluster config at the moment of the split. Happy to revisit if you read that differently.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227363684
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -46,6 +46,7 @@ public class ForwardIndexConfig extends IndexConfig {
private static String _defaultTargetMaxChunkSize = "1MB";
private static int _defaultTargetMaxChunkSizeBytes = 1024 * 1024;
private static int _defaultTargetDocsPerChunk = 1000;
+ private static ChunkCompressionType _defaultCompressionType = ChunkCompressionType.LZ4;
Review Comment:
Done — `volatile` added. Agreed on the asymmetry with the siblings: a stale read of a chunk-size tweak is a performance wobble, a stale read here changes what lands on disk. Noted in the field's javadoc why it differs from its neighbours.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4229534557
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved. The warning names the codecs that are
+ /// accepted, so a rejected value is self-correcting from the log.
+ ///
+ /// Scope, which this being a *cluster* config does not make as wide as it sounds:
+ ///
+ /// - It is read once per JVM at startup, so after changing it a rolling restart leaves restarted
+ /// instances writing the new codec while the rest keep the old one until they restart too. Each
+ /// segment records the codec it was written with, so this mixes formats without breaking reads.
+ /// - It reaches only components that call [#applyClusterConfig]: the controller, broker, server and
+ /// minion. Segments built outside the cluster -- standalone, Spark or Hadoop batch ingestion, and
+ /// `pinot-admin CreateSegment` -- keep the compiled-in default, and because a reload ignores
+ /// defaults, nothing later reconciles the two.
+ /// - It is a process-global default, so in a single-JVM multi-role deployment the last role to start
+ /// wins for the whole process.
+ ///
+ /// These all hold for the three forward index defaults above as well; they are spelled out here
+ /// because this is the first of them an operator is routinely expected to set.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {}, expected one of: {}, keeping: {}",
+ defaultCompressionCodec, rawIndexCompressionCodecs(), ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ if (!codec.isApplicableToRawIndex()) {
Review Comment:
test-auth-probe
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved. The warning names the codecs that are
+ /// accepted, so a rejected value is self-correcting from the log.
+ ///
+ /// Scope, which this being a *cluster* config does not make as wide as it sounds:
+ ///
+ /// - It is read once per JVM at startup, so after changing it a rolling restart leaves restarted
+ /// instances writing the new codec while the rest keep the old one until they restart too. Each
+ /// segment records the codec it was written with, so this mixes formats without breaking reads.
+ /// - It reaches only components that call [#applyClusterConfig]: the controller, broker, server and
+ /// minion. Segments built outside the cluster -- standalone, Spark or Hadoop batch ingestion, and
+ /// `pinot-admin CreateSegment` -- keep the compiled-in default, and because a reload ignores
+ /// defaults, nothing later reconciles the two.
+ /// - It is a process-global default, so in a single-JVM multi-role deployment the last role to start
+ /// wins for the whole process.
+ ///
+ /// These all hold for the three forward index defaults above as well; they are spelled out here
+ /// because this is the first of them an operator is routinely expected to set.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {}, expected one of: {}, keeping: {}",
+ defaultCompressionCodec, rawIndexCompressionCodecs(), ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ if (!codec.isApplicableToRawIndex()) {
Review Comment:
test-auth-probe
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19797:
URL: https://github.com/apache/pinot/pull/19797#issuecomment-6065605767
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19797?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `71.87500%` with `9 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 58.30%. Comparing base ([`f818bae`](https://app.codecov.io/gh/apache/pinot/commit/f818bae6d75a91228eb3aac95c921bca4aeb2128?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8c478bd`](https://app.codecov.io/gh/apache/pinot/commit/8c478bd6c72ca101626af84545c492cce488533b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19797?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ache/pinot/common/utils/ServiceStartableUtils.java](https://app.codecov.io/gh/apache/pinot/pull/19797?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Futils%2FServiceStartableUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvU2VydmljZVN0YXJ0YWJsZVV0aWxzLmphdmE=) | 66.66% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19797?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (f818bae) and HEAD (8c478bd). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (f818bae) | HEAD (8c478bd) |
>|------|------|------|
>|lane-a|2|0|
>|integration|4|0|
>|java-25|6|1|
>|integration1|2|0|
>|temurin|6|1|
>|lane-b|2|0|
>|unittests2|1|0|
>|unittests|2|1|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19797 +/- ##
=============================================
- Coverage 68.55% 58.30% -10.25%
+ Complexity 1486 1 -1485
=============================================
Files 3525 2712 -813
Lines 229852 168238 -61614
Branches 36507 27508 -8999
=============================================
- Hits 157568 98091 -59477
- Misses 59965 61940 +1975
+ Partials 12319 8207 -4112
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.30% <71.87%> (-10.25%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.30% <71.87%> (-10.25%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.30% <71.87%> (-10.25%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.30% <71.87%> (-0.01%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19797?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227373790
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
Review Comment:
Verified — `applyClusterConfig` rewrites `pinot.all.X` to `pinot.<role>.X` (L89-96), and `initForwardIndexConfig` reads the bare key, so the role-prefixed spelling never matches and no warn path fires.
I have not changed it here, deliberately: reading the role-prefixed spelling for this one key would make it the only one of the four that does, which trades an invisible trap for an inconsistent one. It seems better fixed for all four together — either by having `initForwardIndexConfig` check the role-prefixed key as well, or by warning when a `pinot.<role>.forward.index.default.*` key is present but unread. Happy to do that as a follow-up, or in this PR if you would rather it not ship with the gap.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {}, expected one of: {}, keeping: {}",
Review Comment:
The contradiction is fair, and I would rather resolve it by making the siblings lenient than by making this one throw.
A cluster config that throws on a typo takes down every component that restarts after the value is saved, including ones that were healthy — that is a worse failure than building LZ4 segments, and it is why I did not copy the numeric defaults' behaviour. The precedent in this class is `initFieldSpecConfig`, which warn-and-ignores.
On discoverability: the warning now names the accepted codecs, so a typo'd value logs `Unknown forward index default compression codec: ZSTD, expected one of: [PASS_THROUGH, SNAPPY, ZSTANDARD, LZ4, GZIP], keeping: LZ4`. That is self-correcting from the log without needing to inspect segment metadata.
I did not alias `ZSTD`. The value deliberately names a `CompressionCodec`, which is the vocabulary a table config uses, and `ZSTD` is not one — aliasing it here would mean `compressionCodec: ZSTD` works as a cluster default but not in a table config. If you would prefer the alias anyway I will add it, but it seemed worth keeping one spelling per concept.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
Review Comment:
Documented. The method javadoc now states that the value is read once per JVM at startup, so after changing it a rolling restart leaves restarted instances writing the new codec while the rest keep the old one until they restart too — and that this mixes formats without breaking reads, since each segment records the codec it was written with.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
Review Comment:
Fair inconsistency to point out. I kept the lenient parse here: this value is typed by operators into a cluster config, where `snappy` is a very natural thing to write, and the strict alternative silently ignores it.
That argues for making `initFieldSpecConfig` lenient too rather than making this strict, but changing how an existing key parses is a behaviour change to someone else's config and felt out of scope for this PR. If you would like one shared lenient enum-parse helper used by both, say the word and I will add it here.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4229542356
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved. The warning names the codecs that are
+ /// accepted, so a rejected value is self-correcting from the log.
+ ///
+ /// Scope, which this being a *cluster* config does not make as wide as it sounds:
+ ///
+ /// - It is read once per JVM at startup, so after changing it a rolling restart leaves restarted
+ /// instances writing the new codec while the rest keep the old one until they restart too. Each
+ /// segment records the codec it was written with, so this mixes formats without breaking reads.
+ /// - It reaches only components that call [#applyClusterConfig]: the controller, broker, server and
+ /// minion. Segments built outside the cluster -- standalone, Spark or Hadoop batch ingestion, and
+ /// `pinot-admin CreateSegment` -- keep the compiled-in default, and because a reload ignores
+ /// defaults, nothing later reconciles the two.
+ /// - It is a process-global default, so in a single-JVM multi-role deployment the last role to start
+ /// wins for the whole process.
+ ///
+ /// These all hold for the three forward index defaults above as well; they are spelled out here
+ /// because this is the first of them an operator is routinely expected to set.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {}, expected one of: {}, keeping: {}",
+ defaultCompressionCodec, rawIndexCompressionCodecs(), ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ if (!codec.isApplicableToRawIndex()) {
+ LOGGER.warn("Forward index default compression codec: {} is not applicable to raw forward indexes, expected "
+ + "one of: {}, keeping: {}", codec, rawIndexCompressionCodecs(),
+ ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ // Every raw-applicable codec has a same-named ChunkCompressionType; if that ever stops holding, this
+ // should fail loudly rather than silently keep the old default.
+ ChunkCompressionType compressionType = ChunkCompressionType.valueOf(codec.name());
Review Comment:
**[severity: low] This is now the 4th copy of the `CompressionCodec` -> `ChunkCompressionType` mapping, and the only load-bearing one that is name-based.**
The canonical mapping is the explicit switch in `ForwardIndexConfig`'s `@JsonCreator` (where CLP* map to PASS_THROUGH and MV_ENTRY_DICT to null — i.e. it is deliberately *not* name-based); `BaseSingleTreeBuilder` and `CodecPipelineIntegrationTest` carry two more. A by-name `valueOf` still compiles when either enum changes and only fails at runtime — at startup, in this case. Extracting the existing switch into a static `ForwardIndexConfig` helper and calling it here (and from the constructor) keeps one authoritative mapping and gives this call the same fail-loudly behavior the comment asks for.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
Review Comment:
**[severity: low] Doc inaccuracy: `DELTA` is not a "whole-index or dictionary format".**
`DELTA`/`DELTADELTA` are chunk codecs (`ChunkCompressionType.DELTA(6)`, `DELTADELTA(7)`); they are rejected here because table-config validation rejects them everywhere and their compressors throw on chunks whose size is not a multiple of 4 — which the test class doc states correctly. Also `DELTADELTA` is missing from the enumerated list. Suggest splitting the sentence: CLP family / `MV_ENTRY_DICT` are whole-index or dictionary formats; `DELTA`/`DELTADELTA` are chunk codecs that are not valid for general raw columns.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -76,6 +77,23 @@ public static void setDefaultTargetDocsPerChunk(int defaultTargetDocsPerChunk) {
_defaultTargetDocsPerChunk = defaultTargetDocsPerChunk;
}
+ /// The chunk compression type for a raw column whose table config does not name one. Defaults to LZ4.
+ ///
+ /// This is the raw value: the exemption that keeps metrics uncompressed is applied by the caller, in
+ /// [org.apache.pinot.segment.local.segment.index.forward.ForwardIndexType], so a caller resolving a
+ /// default for a specific column must go through that rather than reading this directly.
+ ///
+ /// `volatile` because it is written by the starter thread and read later by segment-build threads: the
+ /// thread-start happens-before covers today's call order, but a stale read here would silently change
Review Comment:
**[severity: low] The "thread-start happens-before covers today's call order" claim doesn't hold for all launch paths.**
`StartServiceManagerCommand.startBootstrapServicesInParallel` runs BROKER/SERVER/MINION `init()` on sibling threads concurrently, `QuickstartRunner.startAll` starts the server before the minion's `init()`, and the PinotServiceManager REST endpoint adds roles to an already-running JVM — in those paths a later role's `applyClusterConfig` runs after segment-build threads exist, with no thread-start edge. So the `volatile` is required today rather than defensive, "last role to start wins" is nondeterministic under parallel bootstrap, and the closing claim in `ServiceStartableUtils` that "these all hold for the three forward index defaults above as well" overstates things — those siblings are non-volatile (and `setDefaultTargetMaxChunkSize` writes its string/bytes pair as two separate stores). Suggest softening the sentence; the `volatile` itself is the right call.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227230370
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
Review Comment:
test-comment-dry-run
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4229539647
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -76,6 +77,23 @@ public static void setDefaultTargetDocsPerChunk(int defaultTargetDocsPerChunk) {
_defaultTargetDocsPerChunk = defaultTargetDocsPerChunk;
}
+ /// The chunk compression type for a raw column whose table config does not name one. Defaults to LZ4.
+ ///
+ /// This is the raw value: the exemption that keeps metrics uncompressed is applied by the caller, in
+ /// [org.apache.pinot.segment.local.segment.index.forward.ForwardIndexType], so a caller resolving a
+ /// default for a specific column must go through that rather than reading this directly.
+ ///
+ /// `volatile` because it is written by the starter thread and read later by segment-build threads: the
+ /// thread-start happens-before covers today's call order, but a stale read here would silently change
+ /// what gets written to disk.
+ public static ChunkCompressionType getDefaultCompressionType() {
+ return _defaultCompressionType;
+ }
+
+ public static void setDefaultCompressionType(ChunkCompressionType defaultCompressionType) {
Review Comment:
**[severity: medium] The public setter accepts values the feature's own guard exists to keep out.**
`setDefaultCompressionType` is public API in `pinot-segment-spi` and accepts `null`, `DELTA`, `DELTADELTA` and `LZ4_LENGTH_PREFIXED`, while the `isApplicableToRawIndex()` validation lives only in `ServiceStartableUtils` (pinot-common) — a module embedders and batch-ingestion plugins don't go through. The PR description itself calls the guard load-bearing: a `DELTA` default makes every raw var-width column fail at build time (`DeltaCompressor` throws on chunks whose size is not a multiple of 4), and `null` NPEs in `ChunkCompressorFactory.getCompressor`.
Consider moving the validate-and-map step into `ForwardIndexConfig` (e.g. a `setDefaultCompressionCodec(CompressionCodec)` that checks `isApplicableToRawIndex()`), leaving `ServiceStartableUtils` to only parse the string. That also removes the cross-module split where the class owning the default doesn't own its invariant.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -567,7 +567,8 @@ private void writeDenseKeyColumn(String key)
// Reconcile dictionary + forward encoding with the final decision (mirrors BaseSegmentCreator.adaptConfig);
// ForwardIndexCreatorFactory selects dict-vs-raw from the forward config's EncodingType. A compression codec
- // applies only to the raw forward format (LZ4 preserves the dense child's current on-disk layout); attaching
+ // applies only to the raw forward format (LZ4 preserves the dense child's current on-disk layout, and is
+ // pinned here regardless of the cluster-wide default compression codec); attaching
Review Comment:
**[severity: medium] OPEN_STRUCT keys now get different codecs depending on how the segment was produced.**
This comment pins LZ4 on the build path, but the reload path is not pinned: materialized keys with no explicit `indexes.forward` node resolve through `FieldIndexConfigsUtil.forwardConfig` -> `ForwardIndexConfig.getDefault(RAW)` (null codec), so a DICT->RAW conversion on reload (`ForwardIndexHandler.rewriteDictToRawForwardIndex` -> `ForwardIndexCreatorFactory`) writes the *configured* cluster default. With `pinot.forward.index.default.compression.codec=ZSTANDARD` and an OPEN_STRUCT table switching `defaultValueFieldConfig` to RAW, existing segments convert keys to ZSTANDARD on reload while freshly built/committed segments write LZ4 — the same key ends up with different codecs per segment, and the "keys are always LZ4 raw" statements in `OpenStructIndexType`/`OpenStructIndexConfig` docs become false.
Nothing breaks on read, but if LZ4 is meant to be pinned for these synthetic columns it should be pinned on both paths (or the splitter should follow the default on both).
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved. The warning names the codecs that are
+ /// accepted, so a rejected value is self-correcting from the log.
+ ///
+ /// Scope, which this being a *cluster* config does not make as wide as it sounds:
+ ///
+ /// - It is read once per JVM at startup, so after changing it a rolling restart leaves restarted
+ /// instances writing the new codec while the rest keep the old one until they restart too. Each
+ /// segment records the codec it was written with, so this mixes formats without breaking reads.
+ /// - It reaches only components that call [#applyClusterConfig]: the controller, broker, server and
+ /// minion. Segments built outside the cluster -- standalone, Spark or Hadoop batch ingestion, and
+ /// `pinot-admin CreateSegment` -- keep the compiled-in default, and because a reload ignores
+ /// defaults, nothing later reconciles the two.
Review Comment:
**[severity: low] "nothing later reconciles the two" is not quite true — some reloads do apply the current default.**
`ForwardIndexHandler` ignores the default only when *deciding whether* to rewrite. Any reload that rebuilds a raw forward index for another reason applies the reloading server's current default via `ForwardIndexCreatorFactory`: DICT->RAW conversion, codecSpec removal, re-enabling a disabled forward index, and new derived/default columns (`BaseDefaultColumnHandler`). So Spark-built LZ4 segments *are* partially reconciled when such an operation runs — and during a rolling restart, two replicas performing the same rewrite can produce locally differing codecs for the same segment. Worth tightening this sentence (the wording in `ForwardIndexType`'s doc, "when deciding whether a reload must rewrite", is the accurate one).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227230370
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
Review Comment:
**[correctness] Multi-role single-JVM deployments: the last starter to run silently overwrites the JVM-global default for all roles.**
`ForwardIndexConfig.setDefaultCompressionType` mutates a process-global static, and every role starter (controller/broker/server/minion) re-runs `initForwardIndexConfig` from its own merged config. In a single-JVM deployment (quickstart, `ServiceManager`), `addConfigIfNotExists` gives each component's local conf precedence over the ZK value, so if the roles' effective values differ (e.g. server conf sets `ZSTANDARD`, minion conf omits it while ZK says `LZ4`), the last-started role flips the shared static *after* earlier roles have begun building segments. A segment build that straddles the flip can even get mixed codecs across columns, since `ForwardIndexCreatorFactory` resolves the default per column at creator-construction time. The three sibling defaults share this mechanism, but this PR adds the first value operators are explicitly invited to set per-instance.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -3024,6 +3024,8 @@ public static class ForwardIndexConfigs {
"pinot.forward.index.default.target.max.chunk.size";
public static final String CONFIG_OF_DEFAULT_TARGET_DOCS_PER_CHUNK =
"pinot.forward.index.default.target.docs.per.chunk";
+ public static final String CONFIG_OF_DEFAULT_COMPRESSION_CODEC =
+ "pinot.forward.index.default.compression.codec";
Review Comment:
**[scope gap] Segment builds outside the cluster never see this config — tables end up permanently mixed-compression.**
Only the four in-cluster starters call `ServiceStartableUtils.applyClusterConfig`. Standalone/Spark/Hadoop batch ingestion (`SegmentGenerationTaskRunner`) and `pinot-admin CreateSegment` build segments in their own JVM through the same `ForwardIndexCreatorFactory` null-codec path, so they keep the compiled-in LZ4 while server/minion-built segments of the same table use the configured codec. Because `shouldRewriteRawForwardIndex` deliberately ignores defaults, reload never reconciles them. The same gap exists for the sibling forward-index defaults, but the new doc on `ForwardIndexType.getDefaultCompressionType` ("unless `pinot.forward.index.default.compression.codec` says otherwise") implies cluster-wide reach it doesn't have — worth a doc note here stating the config only affects components that apply cluster config at startup.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
Review Comment:
**[operator trap] The documented `pinot.all.` spelling of this key is silently dropped.**
`applyClusterConfig`'s javadoc advertises that `pinot.all.*` cluster keys are rewritten to role-specific keys (`pinot.server.forward.index.default.compression.codec`, ...), but `initForwardIndexConfig` reads only the bare key `pinot.forward.index.default.compression.codec`. An operator who follows the documented convention and sets `pinot.all.forward.index.default.compression.codec=ZSTANDARD` gets no warning at all (the warn paths in `setDefaultCompressionCodec` never run because the bare key is absent), and the cluster keeps building LZ4. Pre-existing for the sibling keys, but the new key inherits the trap and it is invisible by design — consider also checking the role-prefixed spelling or logging when it is present.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -335,11 +335,20 @@ private ForwardIndexConfig createConfigFromFieldConfig(FieldConfig fieldConfig,
return builder.build();
}
+ /// The compression type used for a raw column whose table config does not name one.
+ ///
+ /// Metrics stay uncompressed: they are usually small fixed-width values where compression costs more
+ /// than it saves. Everything else uses [ForwardIndexConfig#getDefaultCompressionType()], which is LZ4
+ /// unless `pinot.forward.index.default.compression.codec` says otherwise.
+ ///
+ /// This is a default, not a table config value, so changing it does not rewrite existing segments:
+ /// `ForwardIndexHandler` deliberately ignores it when deciding whether a reload must rewrite a forward
+ /// index.
public static ChunkCompressionType getDefaultCompressionType(FieldSpec.FieldType fieldType) {
if (fieldType == FieldSpec.FieldType.METRIC) {
return ChunkCompressionType.PASS_THROUGH;
} else {
- return ChunkCompressionType.LZ4;
+ return ForwardIndexConfig.getDefaultCompressionType();
Review Comment:
**[behavior change] Columns declared via legacy `noDictionaryConfig: {"col": "SNAPPY"}` now follow the mutable cluster default instead of deterministic LZ4.**
`noDictionaryConfig` values are consumed by `keySet()` only (this file L239-241, `DictionaryIndexType` L127-128) — the per-column codec value is never parsed into the `ForwardIndexConfig`, so such columns carry a null chunk compression type and get filled at `ForwardIndexCreatorFactory:107`. Before this PR that fill-in was the constant LZ4; now it is whatever the cluster default is set to. So a user's *explicit* legacy `"SNAPPY"` is overridden by the cluster knob, and `shouldRewriteRawForwardIndex` (null `newCompressionType`) will never rewrite it back. The value-ignoring is pre-existing, but this PR changes the substitute from deterministic to cluster-mutable — either honor the legacy value or document the interaction.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -335,11 +335,20 @@ private ForwardIndexConfig createConfigFromFieldConfig(FieldConfig fieldConfig,
return builder.build();
}
+ /// The compression type used for a raw column whose table config does not name one.
+ ///
+ /// Metrics stay uncompressed: they are usually small fixed-width values where compression costs more
+ /// than it saves. Everything else uses [ForwardIndexConfig#getDefaultCompressionType()], which is LZ4
Review Comment:
**[missed call sites] "Everything else uses the default" is not true for OPEN_STRUCT-derived columns — `OpenStructColumnSplitter` still hardcodes LZ4.**
`OpenStructColumnSplitter` pins `CompressionCodec.LZ4` for materialized raw key columns (~L575) and `ChunkCompressionType.LZ4` for the sparse remainder column (~L767). With a cluster default of e.g. ZSTANDARD, the dense child is originally written with ZSTANDARD (via this default), then the split re-encodes it as LZ4 — the opposite of the adjacent rationale comment "LZ4 preserves the dense child's current on-disk layout", which assumed LZ4 *was* the default. The precondition message there ("always use a dictionary-encoded or LZ4 raw forward index") also becomes inaccurate. Either route those sites through `ForwardIndexType.getDefaultCompressionType(fieldType)` / leave the codec null, or update the comments to say "LZ4 regardless of the cluster default".
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {}, expected one of: {}, keeping: {}",
Review Comment:
**[silent misconfiguration + policy asymmetry] A near-miss spelling keeps LZ4 forever with only a startup WARN, while the sibling configs three blocks above crash on a typo.**
`ZSTD` — the spelling of the codecSpec DSL and of the zstd tool itself, explicitly anticipated by this PR's own `unrecognisedValues` test — is warn-and-ignored, so the operator sees every component start cleanly, believes ZSTANDARD is active, and only discovers otherwise by inspecting segment metadata later. There is no way to retroactively distinguish "default never applied" from "tables overrode it". Meanwhile a typo in `pinot.forward.index.default.target.docs.per.chunk` still throws from the uncaught `Integer.parseInt` and takes down every restarting component — the exact risk the doc comment above cites as justification for *not* throwing here. One policy for all four defaults (uniformly lenient-with-warn, or fail-fast plus controller-side validation), or at least aliasing `ZSTD`, would remove both the trap and the contradiction.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -76,6 +77,16 @@ public static void setDefaultTargetDocsPerChunk(int defaultTargetDocsPerChunk) {
_defaultTargetDocsPerChunk = defaultTargetDocsPerChunk;
}
+ /// The chunk compression type applied to a raw column whose table config does not name one, except
Review Comment:
**[doc contract] This javadoc promises a metric exemption the method does not implement.**
"...except for metrics, which stay uncompressed" is enforced only in `ForwardIndexType.getDefaultCompressionType(FieldType)` in pinot-segment-local — a module this SPI getter cannot reference. The getter itself returns the configured codec for every field type. A future caller in segment-spi or below segment-local that trusts this doc and calls the getter directly for a metric column would start compressing metrics once the cluster default is set, violating the invariant the PR's own `testConfiguredDefaultDoesNotAffectMetric` pins down. Reword to state that the metric exemption is applied by the caller (`ForwardIndexType`), not by this method.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #19797:
URL: https://github.com/apache/pinot/pull/19797
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227239837
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {}, expected one of: {}, keeping: {}",
+ defaultCompressionCodec, rawIndexCompressionCodecs(), ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ if (!codec.isApplicableToRawIndex()) {
+ LOGGER.warn("Forward index default compression codec: {} is not applicable to raw forward indexes, expected "
+ + "one of: {}, keeping: {}", codec, rawIndexCompressionCodecs(),
+ ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ ChunkCompressionType compressionType;
+ try {
+ compressionType = ChunkCompressionType.valueOf(codec.name());
Review Comment:
**[dead code / duplicated mapping] This try/catch is unreachable, and it is the third copy of the CompressionCodec-to-ChunkCompressionType mapping.**
After `isApplicableToRawIndex()` the codec is one of PASS_THROUGH/SNAPPY/ZSTANDARD/LZ4/GZIP, all of which have same-named `ChunkCompressionType` constants — so the catch can never fire and can never be covered by a test. The canonical mapping already lives in `ForwardIndexConfig`'s constructor switch (and `BaseSingleTreeBuilder:587` does the same by-name conversion unguarded). The canonical map already has a non-identity case (CLP* -> PASS_THROUGH), so if a future raw-applicable codec is spelled differently, the constructor switch gets updated while this warn-and-ignore path silently rejects the cluster default. Prefer the bare `valueOf` (letting drift fail loudly) or one shared conversion helper used by both sites.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -46,6 +46,7 @@ public class ForwardIndexConfig extends IndexConfig {
private static String _defaultTargetMaxChunkSize = "1MB";
private static int _defaultTargetMaxChunkSizeBytes = 1024 * 1024;
private static int _defaultTargetDocsPerChunk = 1000;
+ private static ChunkCompressionType _defaultCompressionType = ChunkCompressionType.LZ4;
Review Comment:
**[latent visibility] Non-volatile mutable static written by the starter thread and read by segment-build/reload threads.**
Publication currently relies only on the thread-start happens-before (starters call `applyClusterConfig` before spawning Helix/segment threads), which matches the four pre-existing sibling fields — but a stale read here silently changes segment output rather than a chunk-size tweak. `volatile` closes it for free:
```suggestion
private static volatile ChunkCompressionType _defaultCompressionType = ChunkCompressionType.LZ4;
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexDefaultCompressionTypeTest.java:
##########
@@ -0,0 +1,67 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.segment.local.segment.index.forward;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.apache.pinot.spi.data.FieldSpec.FieldType;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Tests that the configurable default compression type is honoured for raw columns whose table config
+/// does not name a codec, and that metrics keep their uncompressed default regardless.
+public class ForwardIndexDefaultCompressionTypeTest {
+
+ @AfterMethod(alwaysRun = true)
+ public void tearDown() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @Test
+ public void testDefaultIsLZ4ForNonMetric() {
Review Comment:
**[test robustness] `testDefaultIsLZ4ForNonMetric` asserts the JVM-global initial value but the class has no `@BeforeMethod` reset (only `@AfterMethod`).**
It passes in Maven only because the root surefire runs `reuseForks=false`; in a shared-JVM run (IDE "run all in package", a future surefire change, or any earlier test that sets the static without an `alwaysRun` cleanup — e.g. its own tearDown skipped by a config-method failure) this test fails order-dependently with `ZSTANDARD != LZ4` and cannot be reproduced in isolation. `ServiceStartableUtilsTest` in this same PR already uses the `@BeforeMethod` + `@AfterMethod` pattern — mirroring it here is one line.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/ServiceStartableUtilsTest.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.common.utils;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Covers parsing of the `pinot.forward.index.default.compression.codec` cluster config.
+///
+/// The value names a [org.apache.pinot.spi.config.table.FieldConfig.CompressionCodec], the same
+/// vocabulary a table config uses, and anything that cannot serve as a raw forward index chunk codec is
+/// ignored rather than applied: it would otherwise be injected after table-config validation has run and
+/// fail at segment build time, across every table at once.
+public class ServiceStartableUtilsTest {
+
+ @BeforeMethod
+ public void setUp() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @AfterMethod(alwaysRun = true)
+ public void tearDown() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @DataProvider
+ public Object[][] applicableCodecs() {
+ return new Object[][]{
+ {"ZSTANDARD", ChunkCompressionType.ZSTANDARD},
+ {"zstandard", ChunkCompressionType.ZSTANDARD},
+ {" ZSTANDARD ", ChunkCompressionType.ZSTANDARD},
+ {"SNAPPY", ChunkCompressionType.SNAPPY},
+ {"GZIP", ChunkCompressionType.GZIP},
+ {"PASS_THROUGH", ChunkCompressionType.PASS_THROUGH},
+ {"LZ4", ChunkCompressionType.LZ4}
Review Comment:
**[vacuous test row] The `{"LZ4", ChunkCompressionType.LZ4}` row cannot detect a no-op.**
`setUp()` already sets the default to LZ4 before every method, so this row passes even if `setDefaultCompressionCodec("LZ4")` silently does nothing (e.g. a future refactor that ignores LZ4). Seeding a non-LZ4 value before the call — as `testAnIgnoredValueDoesNotUndoAnEarlierValidOne` does — would make the row meaningful.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
Review Comment:
**[doc gap] The value is applied once per instance at startup, so a rolling restart leaves instances with different defaults.**
After changing the ZK cluster config, restarted servers/minions write the new codec while not-yet-restarted ones keep the old one, so realtime commits and minion rewrites for the same table mix codecs during the rollout window (and a server crash-restarting weeks later silently picks up the new default where its peers did not). No correctness break — each segment records its actual codec — but these per-instance-startup semantics are not documented for this key, and operators generally expect cluster configs to apply cluster-wide at once. Worth a sentence in the method doc and on `CONFIG_OF_DEFAULT_COMPRESSION_CODEC`.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
Review Comment:
**[consistency] New lenient parse (trim + uppercase) diverges from the strict enum parse used for the other enum-valued cluster config in this same file.**
`initFieldSpecConfig` parses `MaxLengthExceedStrategy` with a bare case-sensitive `valueOf` while this key accepts `"snappy"` / `" SNAPPY "`. Two enum-valued cluster configs in one class now follow different spelling rules, so an operator who learns the lenient spelling from one gets a silent ignore (or a crash) from the other. Either keep strict `valueOf` here too, or extract one lenient enum-parse helper and use it for both.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/ServiceStartableUtilsTest.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.common.utils;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Covers parsing of the `pinot.forward.index.default.compression.codec` cluster config.
+///
+/// The value names a [org.apache.pinot.spi.config.table.FieldConfig.CompressionCodec], the same
+/// vocabulary a table config uses, and anything that cannot serve as a raw forward index chunk codec is
+/// ignored rather than applied: it would otherwise be injected after table-config validation has run and
+/// fail at segment build time, across every table at once.
+public class ServiceStartableUtilsTest {
Review Comment:
**[convention] CLAUDE.md: "Add class-level Javadoc for new classes; describe behavior and thread-safety."**
Both new test classes (this one and `ForwardIndexDefaultCompressionTypeTest`) have class docs describing behavior but say nothing about thread-safety — which is material here, because every test method mutates the process-global `ForwardIndexConfig` static, making these classes unsafe to run in parallel with each other or with anything that builds segments. A one-line note ("mutates JVM-global ForwardIndexConfig defaults; must not run in parallel with other users of that static") is exactly what would warn whoever later enables parallel TestNG execution.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4244899622
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved. The warning names the codecs that are
+ /// accepted, so a rejected value is self-correcting from the log.
+ ///
+ /// Scope, which this being a *cluster* config does not make as wide as it sounds:
+ ///
+ /// - It is read once per JVM at startup, so after changing it a rolling restart leaves restarted
+ /// instances writing the new codec while the rest keep the old one until they restart too. Each
+ /// segment records the codec it was written with, so this mixes formats without breaking reads.
+ /// - It reaches only components that call [#applyClusterConfig]: the controller, broker, server and
+ /// minion. Segments built outside the cluster -- standalone, Spark or Hadoop batch ingestion, and
+ /// `pinot-admin CreateSegment` -- keep the compiled-in default, and because a reload ignores
+ /// defaults, nothing later reconciles the two.
+ /// - It is a process-global default, so in a single-JVM multi-role deployment the last role to start
+ /// wins for the whole process.
+ ///
+ /// These all hold for the three forward index defaults above as well; they are spelled out here
+ /// because this is the first of them an operator is routinely expected to set.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {}, expected one of: {}, keeping: {}",
+ defaultCompressionCodec, rawIndexCompressionCodecs(), ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ if (!codec.isApplicableToRawIndex()) {
Review Comment:
Verified and fixed. `GzipDecompressor` holds one `Inflater` as an instance field with an unsynchronized `reset()` / `setInput()` / `inflate()` sequence, and `VarByteChunkForwardIndexReaderV4` builds one decompressor per reader (L75) then passes that same instance into every `CompressedReaderContext` (L119) — so the contexts share it, exactly as you describe.
`ForwardIndexConfig.setDefaultCompressionCodec` now rejects `GZIP` outright, with the reasoning in its javadoc: naming it on one column is an existing opt-in exposure, making it the default would put every raw non-metric column on that path. Covered by `testGzipIsRejectedAsADefault`, and the rejection is in `ForwardIndexConfig` rather than the parser so an embedder setting it directly is guarded too.
I did not fix `GzipDecompressor` itself. A `ThreadLocal` or per-context `Inflater` is the real repair and it fixes a live bug for anyone already using `compressionCodec: GZIP`, but it is a different change with a different blast radius and I would rather it not ride in on a config addition. Happy to raise it separately.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -76,6 +77,23 @@ public static void setDefaultTargetDocsPerChunk(int defaultTargetDocsPerChunk) {
_defaultTargetDocsPerChunk = defaultTargetDocsPerChunk;
}
+ /// The chunk compression type for a raw column whose table config does not name one. Defaults to LZ4.
+ ///
+ /// This is the raw value: the exemption that keeps metrics uncompressed is applied by the caller, in
+ /// [org.apache.pinot.segment.local.segment.index.forward.ForwardIndexType], so a caller resolving a
+ /// default for a specific column must go through that rather than reading this directly.
+ ///
+ /// `volatile` because it is written by the starter thread and read later by segment-build threads: the
+ /// thread-start happens-before covers today's call order, but a stale read here would silently change
+ /// what gets written to disk.
+ public static ChunkCompressionType getDefaultCompressionType() {
+ return _defaultCompressionType;
+ }
+
+ public static void setDefaultCompressionType(ChunkCompressionType defaultCompressionType) {
Review Comment:
Agreed, and moved. Validation now lives in `ForwardIndexConfig.setDefaultCompressionCodec(CompressionCodec)`: it checks `isApplicableToRawIndex()`, rejects `GZIP`, and maps through the shared helper. `ServiceStartableUtils` only parses the string and reports what was rejected.
The unvalidated `setDefaultCompressionType(ChunkCompressionType)` is gone rather than kept alongside it. It was introduced in this PR, so there is no compatibility cost, and leaving it would have preserved exactly the hole you describe — `null`, `DELTA`, `LZ4_LENGTH_PREFIXED` all reachable from outside pinot-common.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -567,7 +567,8 @@ private void writeDenseKeyColumn(String key)
// Reconcile dictionary + forward encoding with the final decision (mirrors BaseSegmentCreator.adaptConfig);
// ForwardIndexCreatorFactory selects dict-vs-raw from the forward config's EncodingType. A compression codec
- // applies only to the raw forward format (LZ4 preserves the dense child's current on-disk layout); attaching
+ // applies only to the raw forward format (LZ4 preserves the dense child's current on-disk layout, and is
+ // pinned here regardless of the cluster-wide default compression codec); attaching
Review Comment:
You are right, and I had only half-fixed this: last round I annotated the splitter's own comment but left `OpenStructIndexType`'s claim standing.
`FIXED_FORWARD_INDEX` now reads "the splitter fixes materialized keys to a dictionary-encoded or LZ4 raw forward index" rather than "materialized keys always use ...", which is the word that stopped being true. The validation comment and the `validatePerKeyForwardIndex` javadoc both say the LZ4 is the splitter's and that a dictionary-to-raw conversion on reload takes the reloading component's default instead. Three test assertions pinned the old message and were updated with it.
Worth noting a second copy of that sentence lived in `OpenStructColumnSplitter` itself, line-split across two string literals, which is why my first pass missed it — the tests caught it.
I have not pinned the reload path. Making it match means the open-struct reload route carrying its codec explicitly, which is a change owned by that feature rather than by this config, and I would rather not silently alter open-struct segment output from here. The regression test you suggest belongs with that change.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved. The warning names the codecs that are
+ /// accepted, so a rejected value is self-correcting from the log.
+ ///
+ /// Scope, which this being a *cluster* config does not make as wide as it sounds:
+ ///
+ /// - It is read once per JVM at startup, so after changing it a rolling restart leaves restarted
+ /// instances writing the new codec while the rest keep the old one until they restart too. Each
+ /// segment records the codec it was written with, so this mixes formats without breaking reads.
+ /// - It reaches only components that call [#applyClusterConfig]: the controller, broker, server and
+ /// minion. Segments built outside the cluster -- standalone, Spark or Hadoop batch ingestion, and
+ /// `pinot-admin CreateSegment` -- keep the compiled-in default, and because a reload ignores
+ /// defaults, nothing later reconciles the two.
Review Comment:
Corrected. The sentence now reads that a reload ignores the default when *deciding* whether to rewrite, so nothing reconciles the two on its own, but that a rewrite triggered for another reason — dictionary-to-raw conversion, codecSpec removal, re-enabling a disabled forward index, a new derived or default column — does write the reloading component's current default.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make the default forward index compression codec configurable [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4244902061
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing would take down every
+ /// component that restarts after the bad value is saved. The warning names the codecs that are
+ /// accepted, so a rejected value is self-correcting from the log.
+ ///
+ /// Scope, which this being a *cluster* config does not make as wide as it sounds:
+ ///
+ /// - It is read once per JVM at startup, so after changing it a rolling restart leaves restarted
+ /// instances writing the new codec while the rest keep the old one until they restart too. Each
+ /// segment records the codec it was written with, so this mixes formats without breaking reads.
+ /// - It reaches only components that call [#applyClusterConfig]: the controller, broker, server and
+ /// minion. Segments built outside the cluster -- standalone, Spark or Hadoop batch ingestion, and
+ /// `pinot-admin CreateSegment` -- keep the compiled-in default, and because a reload ignores
+ /// defaults, nothing later reconciles the two.
+ /// - It is a process-global default, so in a single-JVM multi-role deployment the last role to start
+ /// wins for the whole process.
+ ///
+ /// These all hold for the three forward index defaults above as well; they are spelled out here
+ /// because this is the first of them an operator is routinely expected to set.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec = CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {}, expected one of: {}, keeping: {}",
+ defaultCompressionCodec, rawIndexCompressionCodecs(), ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ if (!codec.isApplicableToRawIndex()) {
+ LOGGER.warn("Forward index default compression codec: {} is not applicable to raw forward indexes, expected "
+ + "one of: {}, keeping: {}", codec, rawIndexCompressionCodecs(),
+ ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ // Every raw-applicable codec has a same-named ChunkCompressionType; if that ever stops holding, this
+ // should fail loudly rather than silently keep the old default.
+ ChunkCompressionType compressionType = ChunkCompressionType.valueOf(codec.name());
Review Comment:
Done. The switch is extracted into `ForwardIndexConfig.toChunkCompressionType` and called from both the `@JsonCreator` constructor and the new setter, so there is one authoritative mapping and it keeps the non-identity cases (CLP family to `PASS_THROUGH`, `MV_ENTRY_DICT` to null) rather than relying on names matching.
The helper is `private`: its only callers are in `ForwardIndexConfig`, and it maps precisely the codecs the setter exists to reject, so exposing it would offer a way around those guards.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by `compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe whole-index or dictionary formats
Review Comment:
Fixed. The sentence now distinguishes the two reasons: the CLP family and `MV_ENTRY_DICT` name whole-index or dictionary-id formats rather than chunk codecs; `DELTA` and `DELTADELTA` *are* chunk codecs, rejected because table config validation rejects them on every column and their compressors throw on chunks that are not a multiple of 4 bytes. `DELTADELTA` is now named alongside `DELTA` everywhere the list appears.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -76,6 +77,23 @@ public static void setDefaultTargetDocsPerChunk(int defaultTargetDocsPerChunk) {
_defaultTargetDocsPerChunk = defaultTargetDocsPerChunk;
}
+ /// The chunk compression type for a raw column whose table config does not name one. Defaults to LZ4.
+ ///
+ /// This is the raw value: the exemption that keeps metrics uncompressed is applied by the caller, in
+ /// [org.apache.pinot.segment.local.segment.index.forward.ForwardIndexType], so a caller resolving a
+ /// default for a specific column must go through that rather than reading this directly.
+ ///
+ /// `volatile` because it is written by the starter thread and read later by segment-build threads: the
+ /// thread-start happens-before covers today's call order, but a stale read here would silently change
Review Comment:
You are right, and the claim is now gone. The field's javadoc says `volatile` is needed because the thread-start happens-before does not cover every launch path, naming `startBootstrapServicesInParallel` (sibling threads per role) and the PinotServiceManager REST endpoint adding a role to a JVM that is already serving.
The "these all hold for the siblings too" sentence is also corrected: the first two caveats hold for them, while the third is *worse* for the siblings, since under a parallel bootstrap the roles race and their statics are not `volatile`.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/ServiceStartableUtilsTest.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.common.utils;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Covers parsing of the `pinot.forward.index.default.compression.codec` cluster config.
+///
+/// The value names a [org.apache.pinot.spi.config.table.FieldConfig.CompressionCodec], the same
+/// vocabulary a table config uses, and anything that cannot serve as a raw forward index chunk codec is
+/// ignored rather than applied: it would otherwise be injected after table-config validation has run and
+/// fail at segment build time, across every table at once.
+///
+/// Not thread-safe: every method mutates the process-global [ForwardIndexConfig] default, so this class
+/// must not run in parallel with other tests, or with anything that builds segments.
+public class ServiceStartableUtilsTest {
+
+ @BeforeMethod
+ public void setUp() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @AfterMethod(alwaysRun = true)
+ public void tearDown() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @DataProvider
+ public Object[][] applicableCodecs() {
+ return new Object[][]{
+ {"ZSTANDARD", ChunkCompressionType.ZSTANDARD},
+ {"zstandard", ChunkCompressionType.ZSTANDARD},
+ {" ZSTANDARD ", ChunkCompressionType.ZSTANDARD},
+ {"SNAPPY", ChunkCompressionType.SNAPPY},
+ {"GZIP", ChunkCompressionType.GZIP},
+ {"PASS_THROUGH", ChunkCompressionType.PASS_THROUGH},
+ {"LZ4", ChunkCompressionType.LZ4}
+ };
+ }
+
+ @Test(dataProvider = "applicableCodecs")
+ public void testApplicableCodecIsApplied(String configured, ChunkCompressionType expected) {
+ // Seed something other than the value under test, so a row whose expectation happens to equal the
+ // starting default -- LZ4 -- still fails if the codec is never applied.
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.GZIP);
+ if (expected == ChunkCompressionType.GZIP) {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.SNAPPY);
+ }
+
+ ServiceStartableUtils.setDefaultCompressionCodec(configured);
+
+ assertEquals(ForwardIndexConfig.getDefaultCompressionType(), expected);
+ }
+
+ /// `DELTA` and `DELTADELTA` matter most here: they are rejected by table-config validation on every
+ /// column, and their compressors throw on any chunk whose size is not a multiple of 4 bytes, so every
+ /// raw var-width column in the cluster would fail to build if one slipped through as a default.
+ @DataProvider
+ public Object[][] codecsNotApplicableToRawIndexes() {
+ return new Object[][]{
+ {"DELTA"}, {"DELTADELTA"}, {"MV_ENTRY_DICT"}, {"CLP"}, {"CLPV2"}, {"CLPV2_ZSTD"}, {"CLPV2_LZ4"}
+ };
+ }
+
+ @Test(dataProvider = "codecsNotApplicableToRawIndexes")
Review Comment:
Fixed, and more broadly than this one test. The class now seeds a SNAPPY baseline in `@BeforeMethod`, so no assertion in it expects its own starting value — the rejection tests and `testUnrecognisedValueKeepsCurrentDefault` included. A regression that wrongly applied LZ4, or mapped a rejected codec to LZ4, now fails.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to: {}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+ instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
Review Comment:
Added, in both directions.
Consumer side: `testTheConfigKeyReachesTheDefault` builds a `PinotConfiguration` holding `CONFIG_OF_DEFAULT_COMPRESSION_CODEC` and drives `initForwardIndexConfig`, with `testAbsentConfigKeyLeavesTheDefaultAlone` for the other branch. `initForwardIndexConfig` is `@VisibleForTesting` for this.
Producer side, which was the larger hole: nothing proved the default actually reached a creator. `ForwardIndexCreatorFactoryTest` now has `testCreatorFollowsTheConfiguredDefaultCompressionType` and `testMetricColumnIgnoresTheConfiguredDefaultCompressionType`, which build a creator from a codec-less config and assert `getRawForwardIndexChunkCompressionType()`. A refactor that inlined LZ4 in the factory would now fail.
I also restored coverage of the compiled-in value itself: `testCompiledInDefaultIsLZ4` captures the default in `@BeforeClass`, before any `@BeforeMethod` sets one, which is what every "segments built outside the cluster keep LZ4" sentence rests on.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexDefaultCompressionTypeTest.java:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.segment.local.segment.index.forward;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.apache.pinot.spi.data.FieldSpec.FieldType;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Tests that the configurable default compression type is honoured for raw columns whose table config
+/// does not name a codec, and that metrics keep their uncompressed default regardless.
+///
+/// Not thread-safe: every method mutates the process-global [ForwardIndexConfig] default, so this class
+/// must not run in parallel with other tests, or with anything that builds segments.
+public class ForwardIndexDefaultCompressionTypeTest {
Review Comment:
Partly taken. The two producer-side cases went into the existing `ForwardIndexCreatorFactoryTest`, next to the assertions that already cover the codec a creator reports, which is where they belong.
I kept `ForwardIndexDefaultCompressionTypeTest` separate. Its lifecycle is not just a one-line reset: it mutates a process-global static in `@BeforeMethod` and `@AfterMethod`, and now also captures the compiled-in value in `@BeforeClass` before anything sets it. Folding that into `ForwardIndexTypeTest` imposes the reset on every unrelated test there and makes the `@BeforeClass` capture depend on no sibling test having moved the static first. The two tests you note as covering pre-existing behavior are gone: the redundant LZ4 one is replaced by the `@BeforeClass` capture, which actually can fail.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org