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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
