klsince commented on code in PR #12440:
URL: https://github.com/apache/pinot/pull/12440#discussion_r1601978383
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/CommonsConfigurationUtilsTest.java:
##########
@@ -50,6 +53,33 @@ public void tearDown()
FileUtils.deleteDirectory(TEMP_DIR);
}
+ @Test
+ public void testSegmentMetadataFromFile() {
+ // load the existing config and check the properties config instance
+ try {
+ PropertiesConfiguration config = CommonsConfigurationUtils
+ .getSegmentMetadataFromFile(SEGMENT_METADATA_CONFIG_FILE, true);
+ assertNotNull(config);
+
+ config.setProperty("testKey", "testValue");
+
+ // add the segment version header to the file and read it again
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(config, CONFIG_FILE,
+ CommonsConfigurationUtils.PROPERTIES_CONFIGURATION_HEADER_VERSION_2);
+
+ // reading the property with header.
+ config =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true);
+ assertNotNull(config);
+ assertEquals(config.getHeader(), "# version=2");
+ } catch (Exception ex) {
+ Assert.fail("should not throw ConfigurationException exception with
valid file");
Review Comment:
put ex.getMessage() in the failure msg to help debug
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/SegmentMetadataPropertyReader.java:
##########
@@ -0,0 +1,54 @@
+/**
+ * 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.spi.env;
+
+import com.google.common.base.Preconditions;
+import java.io.Reader;
+import
org.apache.commons.configuration2.PropertiesConfiguration.PropertiesReader;
+
+
+/**
+ * SegmentMetadataPropertyReader extends the PropertiesReader
+ * <p>
+ * Purpose: loads the segment metadata faster
+ * - by skipping the unescaping of key and
+ * - parsing the line by splitting based on first occurrence of separator
+ */
+class SegmentMetadataPropertyReader extends PropertiesReader {
+
+ public SegmentMetadataPropertyReader(Reader reader) {
+ super(reader);
+ }
+
+ @Override
+ protected void parseProperty(final String line) {
+ // skip the regex based parsing of the line content and splitting the
content based on first occurrence of separator
+ String[] keyValue = line.split(getPropertySeparator());
+ Preconditions.checkArgument(keyValue.length == 2, "property content split
should result in key and value");
+ initPropertyName(keyValue[0]);
+ initPropertyValue(keyValue[1]);
+ initPropertySeparator(getPropertySeparator());
Review Comment:
would separator be something other than '='? if so, I wonder if the split
logic in the util class would fail e.g.
```
String[] headerKeyValue = fileFirstLine.split("=");
```
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/CommonsConfigurationUtilsTest.java:
##########
@@ -50,6 +53,33 @@ public void tearDown()
FileUtils.deleteDirectory(TEMP_DIR);
}
+ @Test
+ public void testSegmentMetadataFromFile() {
+ // load the existing config and check the properties config instance
+ try {
+ PropertiesConfiguration config = CommonsConfigurationUtils
+ .getSegmentMetadataFromFile(SEGMENT_METADATA_CONFIG_FILE, true);
+ assertNotNull(config);
+
+ config.setProperty("testKey", "testValue");
+
+ // add the segment version header to the file and read it again
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(config, CONFIG_FILE,
+ CommonsConfigurationUtils.PROPERTIES_CONFIGURATION_HEADER_VERSION_2);
+
+ // reading the property with header.
+ config =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true);
+ assertNotNull(config);
+ assertEquals(config.getHeader(), "# version=2");
+ } catch (Exception ex) {
+ Assert.fail("should not throw ConfigurationException exception with
valid file");
+ }
+
+ // load the non-existing file and expect the exception
+ Assert.expectThrows(NullPointerException.class,
+ () -> CommonsConfigurationUtils.getSegmentMetadataFromFile(null,
true));
Review Comment:
can add another check for non-existing file, in addition to `null`
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/VersionedPropertyConfigTest.java:
##########
@@ -0,0 +1,190 @@
+/**
+ * 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.spi.env;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import org.apache.commons.configuration2.PropertiesConfiguration;
+import org.apache.commons.configuration2.ex.ConfigurationException;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.lang3.RandomStringUtils;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertNull;
+
+
+public class VersionedPropertyConfigTest {
+ private static final File TEMP_DIR = new File(FileUtils.getTempDirectory(),
"VersionedPropertyConfigTest");
+ private static final File CONFIG_FILE = new File(TEMP_DIR, "config");
+ private static final String[] TEST_PROPERTY_KEY = { "test1", "test2_key",
"test3_key_",
+ "test4_key_1234", "test-1", "test.1" };
+ private static final String[] TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR = {
"test:1", "test2=key",
+ "test3,key_", "test4:=._key_1234", "test5-1=" };
+
+ @BeforeClass
+ public void setUp()
+ throws IOException {
+ FileUtils.deleteDirectory(TEMP_DIR);
+ }
+
+ @AfterClass
+ @AfterMethod
+ public void tearDown()
+ throws IOException {
+ FileUtils.deleteDirectory(TEMP_DIR);
+ }
+
+ @Test
+ public void testSegmentMetadataPropertyConfiguration()
+ throws ConfigurationException {
+ testSegmentMetadataPropertiesConfiguration(null, TEST_PROPERTY_KEY);
Review Comment:
maybe add a test for version1 as well, to check if it's handled as expected
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/VersionedPropertyConfigTest.java:
##########
@@ -0,0 +1,190 @@
+/**
+ * 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.spi.env;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import org.apache.commons.configuration2.PropertiesConfiguration;
+import org.apache.commons.configuration2.ex.ConfigurationException;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.lang3.RandomStringUtils;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertNull;
+
+
+public class VersionedPropertyConfigTest {
+ private static final File TEMP_DIR = new File(FileUtils.getTempDirectory(),
"VersionedPropertyConfigTest");
+ private static final File CONFIG_FILE = new File(TEMP_DIR, "config");
+ private static final String[] TEST_PROPERTY_KEY = { "test1", "test2_key",
"test3_key_",
+ "test4_key_1234", "test-1", "test.1" };
+ private static final String[] TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR = {
"test:1", "test2=key",
+ "test3,key_", "test4:=._key_1234", "test5-1=" };
+
+ @BeforeClass
+ public void setUp()
+ throws IOException {
+ FileUtils.deleteDirectory(TEMP_DIR);
+ }
+
+ @AfterClass
+ @AfterMethod
+ public void tearDown()
+ throws IOException {
+ FileUtils.deleteDirectory(TEMP_DIR);
+ }
+
+ @Test
+ public void testSegmentMetadataPropertyConfiguration()
+ throws ConfigurationException {
+ testSegmentMetadataPropertiesConfiguration(null, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataPropertyConfigurationWithHeader()
+ throws ConfigurationException {
+
testSegmentMetadataPropertiesConfiguration(CommonsConfigurationUtils.PROPERTIES_CONFIGURATION_HEADER_VERSION_2,
+ TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataReaderWithSpecialCharsPropertyKeys()
+ throws ConfigurationException {
+ testSegmentMetadataPropertiesConfiguration(null,
TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR);
+ }
+
+ @Test
+ public void testSegmentMetadataReaderWithSpecialCharsPropertyKeysWithHeader()
+ throws ConfigurationException {
+
testSegmentMetadataPropertiesConfiguration(CommonsConfigurationUtils.PROPERTIES_CONFIGURATION_HEADER_VERSION_2,
+ TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR);
+ }
+
+ @Test
+ //Test requires 'segment-metadata-without-version-header.properties' sample
segment metadata file in resources folder
+ public void testOldSegmentMetadataBackwardCompatability()
+ throws ConfigurationException {
+ File oldSegmentProperties = new File(
+ Objects.requireNonNull(
+ PropertiesConfiguration.class.getClassLoader()
+
.getResource("segment-metadata-without-version-header.properties")).getFile());
+ PropertiesConfiguration configuration =
+
CommonsConfigurationUtils.getSegmentMetadataFromFile(oldSegmentProperties,
true);
+
+ // assert that Header is null for the config.
+ assertNull(configuration.getHeader());
+
+ // assert that configuration has DefaultIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
PropertiesConfiguration.DefaultIOFactory.class);
+
+ testSegmentMetadataContent(configuration);
+
+ // asserting escaped value ('column-ProductId-maxValue')
+ String productIDMaxValue =
configuration.getString("column-ProductId-maxValue");
+ assertNotNull(productIDMaxValue);
+ assertEquals(productIDMaxValue, "B009,WVB40S");
+ }
+
+ @Test
+ //Test requires 'segment-metadata-with-version-header.properties' sample
segment metadata file in resources folder
+ public void testSegmentMetadataWithVersionHeader()
+ throws ConfigurationException {
+ File oldSegmentProperties = new File(
+ Objects.requireNonNull(
+ PropertiesConfiguration.class.getClassLoader()
+
.getResource("segment-metadata-with-version-header.properties")).getFile());
+ PropertiesConfiguration configuration =
+
CommonsConfigurationUtils.getSegmentMetadataFromFile(oldSegmentProperties,
true);
+
+ // assert that Header is equals to '# version=2'
+ assertEquals(configuration.getHeader(), "# version=2");
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
VersionedIOFactory.class);
+
+ testSegmentMetadataContent(configuration);
+ }
+
+ private static void testSegmentMetadataContent(PropertiesConfiguration
configuration) {
+ // getting all the keys, length of the list should be equal to the number
of lines in the segment metadata
+ List<String> keys = CommonsConfigurationUtils.getKeys(configuration);
+ assertEquals(keys.size(), 123);
+
+ // asserting table name property from segment metadata
+ String tableName = configuration.getString("segment.table.name");
+ assertEquals(tableName, "fineFoodReviews");
+
+ // asserting table name property from segment metadata
+ String segmentName = configuration.getString("segment.name");
+ assertEquals(segmentName, "fineFoodReviews_OFFLINE_0");
+
+ // asserting segment dimension column names from segment metadata
+ String[] segmentDimensionColumnNames =
configuration.getStringArray("segment.dimension.column.names");
+ assertEquals(segmentDimensionColumnNames.length, 8);
+ assertEquals(String.join(",", segmentDimensionColumnNames),
+ "ProductId,Score,Summary,Text,UserId,combined,embedding,n_tokens");
+
+ // asserting segment.index.version
+ String segmentIndexVersion =
configuration.getString("segment.index.version");
+ assertEquals(segmentIndexVersion, "v3");
+ }
+
+ private static void testSegmentMetadataPropertiesConfiguration(String
versionHeader, String[] keysArray)
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true);
+
+ // setting the random value of the test keys
+ for (String key: keysArray) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, keysArray);
+
+ // save the configuration.
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, versionHeader);
+
+ if (versionHeader != null) {
+ String expectedHeader =
+ String.format("%s=%s",
CommonsConfigurationUtils.VERSION_HEADER_IDENTIFIER, versionHeader);
+ assertEquals(configuration.getHeader(), expectedHeader);
+ }
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, keysArray);
+ }
+
+ private static void testPropertyKeys(List<String> recoveredKeys, String[]
actualKeys) {
+ assertEquals(recoveredKeys.size(), actualKeys.length);
+ for (int i = 1; i < recoveredKeys.size(); i++) {
Review Comment:
why not start from 0, or use `<=` recoveredKeys.size()?
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
