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: [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]
