klsince commented on code in PR #12440:
URL: https://github.com/apache/pinot/pull/12440#discussion_r1569256706
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -287,4 +337,51 @@ private static PropertiesConfiguration
createPropertiesConfiguration(boolean set
return config;
}
+
+ /**
+ * Creates the IOFactory based on the provided kind.
+ *
+ * @param ioFactoryKind IOFactory kind
+ * @return IOFactory
+ */
+ private static IOFactory createPropertyIOFactory(PropertyIOFactoryKind
ioFactoryKind) {
+ switch (ioFactoryKind) {
+ case ConfigFileIOFactory:
+ return new ConfigFilePropertyIOFactory();
+ case SegmentMetadataIOFactory:
+ return new SegmentMetadataPropertyIOFactory();
+ default:
+ return new PropertiesConfiguration.DefaultIOFactory();
+ }
+ }
+
+ /**
+ * checks whether the configuration file first line is version header or not.
+ * @param file configuration file
+ * @return String
+ * @throws ConfigurationException exception.
+ */
+ private static String getConfigurationHeaderVersion(File file)
+ throws ConfigurationException {
+ String versionValue =
PropertyIOFactoryKind.DefaultPropertyConfigurationIOFactory.getVersion();
+ if (file.exists()) {
+ try (BufferedReader reader = new BufferedReader(new FileReader(file))) {
+ String fileFirstLine = reader.readLine();
+ // header version is written as a comment and start with '# '
+ String versionHeaderCommentPrefix = String.format("# %s",
VERSION_HEADER_IDENTIFIER);
+ // check whether the segment has the version header or not
Review Comment:
nit: s/segment/file, as this method doesn't have the concept of `segment`
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/SegmentMetadataPropertyConfigTest.java:
##########
@@ -0,0 +1,213 @@
+/**
+ * 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 SegmentMetadataPropertyConfigTest {
+ private static final File TEMP_DIR = new File(FileUtils.getTempDirectory(),
"SegmentMetadataPropertyConfigTest");
+ 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 {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataPropertyConfigurationWithHeader()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ // save the configuration.
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE,
+ PropertyIOFactoryKind.SegmentMetadataIOFactory.getVersion());
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
SegmentMetadataPropertyIOFactory.class);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataReaderWithSpecialCharsPropertyKeys()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, 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, 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, true);
+
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
SegmentMetadataPropertyIOFactory.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");
+ assertNotNull(tableName);
+ assertEquals(tableName, "fineFoodReviews");
+
+ // asserting table name property from segment metadata
+ String segmentName = configuration.getString("segment.name");
+ assertNotNull(tableName);
+ assertEquals(segmentName, "fineFoodReviews_OFFLINE_0");
+
+ // asserting segment dimension column names from segment metadata
+ String[] segmentDimensionColumnNames =
configuration.getStringArray("segment.dimension.column.names");
+ assertNotNull(segmentDimensionColumnNames);
Review Comment:
can remove
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/CommonsConfigurationUtilsTest.java:
##########
@@ -50,6 +52,31 @@ 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(CONFIG_FILE, false, true);
+ assertNotNull(config);
+
+ config.setProperty("testKey", "testValue");
+
+ // add the segment version header to the file and read it again
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(config, CONFIG_FILE,
"version1");
+
+ // reading the property with header.
+ config =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, false, true);
+ assertNotNull(config);
+ assertNotNull(config.getHeader());
Review Comment:
assert it's `# version=version1` to be more strict here?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -287,4 +337,51 @@ private static PropertiesConfiguration
createPropertiesConfiguration(boolean set
return config;
}
+
+ /**
+ * Creates the IOFactory based on the provided kind.
+ *
+ * @param ioFactoryKind IOFactory kind
+ * @return IOFactory
+ */
+ private static IOFactory createPropertyIOFactory(PropertyIOFactoryKind
ioFactoryKind) {
+ switch (ioFactoryKind) {
+ case ConfigFileIOFactory:
+ return new ConfigFilePropertyIOFactory();
+ case SegmentMetadataIOFactory:
+ return new SegmentMetadataPropertyIOFactory();
+ default:
+ return new PropertiesConfiguration.DefaultIOFactory();
+ }
+ }
+
+ /**
+ * checks whether the configuration file first line is version header or not.
+ * @param file configuration file
+ * @return String
+ * @throws ConfigurationException exception.
+ */
+ private static String getConfigurationHeaderVersion(File file)
+ throws ConfigurationException {
+ String versionValue =
PropertyIOFactoryKind.DefaultPropertyConfigurationIOFactory.getVersion();
+ if (file.exists()) {
+ try (BufferedReader reader = new BufferedReader(new FileReader(file))) {
+ String fileFirstLine = reader.readLine();
+ // header version is written as a comment and start with '# '
+ String versionHeaderCommentPrefix = String.format("# %s",
VERSION_HEADER_IDENTIFIER);
+ // check whether the segment has the version header or not
+ if (StringUtils.startsWith(fileFirstLine, versionHeaderCommentPrefix))
{
+ String[] headerKeyValue = fileFirstLine.split("=");
+ if (headerKeyValue.length == 2) {
+ versionValue = headerKeyValue[1]; // set version value
Review Comment:
nit: can remove this comment, as this stmt is pretty self explained.
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/SegmentMetadataPropertyConfigTest.java:
##########
@@ -0,0 +1,213 @@
+/**
+ * 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 SegmentMetadataPropertyConfigTest {
+ private static final File TEMP_DIR = new File(FileUtils.getTempDirectory(),
"SegmentMetadataPropertyConfigTest");
+ 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 {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataPropertyConfigurationWithHeader()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ // save the configuration.
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE,
+ PropertyIOFactoryKind.SegmentMetadataIOFactory.getVersion());
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
SegmentMetadataPropertyIOFactory.class);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataReaderWithSpecialCharsPropertyKeys()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, 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, 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, true);
+
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
Review Comment:
assert the value of header?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -128,6 +156,18 @@ public static PropertiesConfiguration fromFile(File file,
boolean setIOFactory,
return config;
}
+ public static void saveSegmentMetadataToFile(PropertiesConfiguration
propertiesConfiguration, File file,
+ String versionHeader) {
+ if (StringUtils.isNotEmpty(versionHeader)) {
Review Comment:
should we also do this check here before setting the IOFactory?
```
if
(PropertyIOFactoryKind.SegmentMetadataIOFactory.getVersion().equals(versionHeader))
{
ioFactoryKind = PropertyIOFactoryKind.SegmentMetadataIOFactory;
}
```
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/SegmentMetadataPropertyConfigTest.java:
##########
@@ -0,0 +1,213 @@
+/**
+ * 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 SegmentMetadataPropertyConfigTest {
+ private static final File TEMP_DIR = new File(FileUtils.getTempDirectory(),
"SegmentMetadataPropertyConfigTest");
+ 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 {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataPropertyConfigurationWithHeader()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ // save the configuration.
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE,
+ PropertyIOFactoryKind.SegmentMetadataIOFactory.getVersion());
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
SegmentMetadataPropertyIOFactory.class);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataReaderWithSpecialCharsPropertyKeys()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, 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, 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, true);
+
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
SegmentMetadataPropertyIOFactory.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");
+ assertNotNull(tableName);
Review Comment:
nit: no need for this with the assertion below
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -54,7 +59,8 @@ private CommonsConfigurationUtils() {
*/
public static PropertiesConfiguration fromFile(File file)
throws ConfigurationException {
- return fromFile(file, false, true);
+ return fromFile(file, false, true,
+ PropertyIOFactoryKind.DefaultPropertyConfigurationIOFactory);
Review Comment:
ic. so if `setIOFactory` is false and we don't do setIOFactory(), is
`PropertiesConfiguration.DefaultIOFactory()` to be used? If so, looks like this
`setIOFactory` boolean param is redundant with `PropertyIOFactoryKind` param
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -100,24 +107,50 @@ public static PropertiesConfiguration fromPath(String
path, boolean setIOFactory
* @return a {@link PropertiesConfiguration} instance.
*/
public static PropertiesConfiguration fromInputStream(InputStream stream,
boolean setIOFactory,
- boolean setDefaultDelimiter)
+ boolean setDefaultDelimiter, PropertyIOFactoryKind ioFactoryKind)
throws ConfigurationException {
- PropertiesConfiguration config =
createPropertiesConfiguration(setIOFactory, setDefaultDelimiter);
+ PropertiesConfiguration config =
createPropertiesConfiguration(setIOFactory, setDefaultDelimiter, ioFactoryKind);
FileHandler fileHandler = new FileHandler(config);
fileHandler.load(stream);
return config;
}
+ /**
+ * Instantiate a Segment Metadata {@link PropertiesConfiguration} from a
{@link File}.
+ * @param file containing properties
+ * @param setIOFactory representing to set the IOFactory or not
+ * @param setDefaultDelimiter representing to set the default list delimiter.
+ * @return a {@link PropertiesConfiguration} instance.
+ */
+ public static PropertiesConfiguration segmentMetadataFromFile(File file,
boolean setIOFactory,
+ boolean setDefaultDelimiter)
+ throws ConfigurationException {
+ PropertyIOFactoryKind ioFactoryKind =
PropertyIOFactoryKind.DefaultPropertyConfigurationIOFactory;
+
+ try {
+ if (checkHeaderExistsInSegmentMetadata(file)) {
+ ioFactoryKind = PropertyIOFactoryKind.SegmentMetadataIOFactory;
+ }
+ } catch (IOException exception) {
+ throw new ConfigurationException(
+ String.format("Error occurred while reading segment metadata file %s
", file.getName()), exception);
+ }
+
+ return fromFile(file, setIOFactory, setDefaultDelimiter, ioFactoryKind);
+ }
+
/**
* Instantiate a {@link PropertiesConfiguration} from a {@link File}.
* @param file containing properties
* @param setIOFactory representing to set the IOFactory or not
* @param setDefaultDelimiter representing to set the default list delimiter.
* @return a {@link PropertiesConfiguration} instance.
*/
- public static PropertiesConfiguration fromFile(File file, boolean
setIOFactory, boolean setDefaultDelimiter)
+ public static PropertiesConfiguration fromFile(File file, boolean
setIOFactory,
+ boolean setDefaultDelimiter, PropertyIOFactoryKind ioFactoryKind)
throws ConfigurationException {
- PropertiesConfiguration config =
createPropertiesConfiguration(setIOFactory, setDefaultDelimiter);
+ Preconditions.checkNotNull(file, "File object can not be null for loading
configurations");
Review Comment:
bump ^
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/SegmentMetadataPropertyConfigTest.java:
##########
@@ -0,0 +1,213 @@
+/**
+ * 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 SegmentMetadataPropertyConfigTest {
+ private static final File TEMP_DIR = new File(FileUtils.getTempDirectory(),
"SegmentMetadataPropertyConfigTest");
+ 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 {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataPropertyConfigurationWithHeader()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ // save the configuration.
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE,
+ PropertyIOFactoryKind.SegmentMetadataIOFactory.getVersion());
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
SegmentMetadataPropertyIOFactory.class);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataReaderWithSpecialCharsPropertyKeys()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
Review Comment:
add a test withHeader?
also since the tests for withHeader and withouHeader are same but there, we
can define a dataProvider to parameterize the test methods to save some codes
here.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/PropertyIOFactoryKind.java:
##########
@@ -0,0 +1,25 @@
+/**
+ * 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;
+
+public enum PropertyIOFactoryKind {
Review Comment:
bump ^
And do we need to define a version number for each IOFactory types? If we
just need version header for segment metadata file, we probably just need to
define version for segmentMetadata IO factory. Personally I'd just define those
as constants in CommonsConfigurationFile class for simplicity.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -271,13 +311,23 @@ public static String
recoverSpecialCharacterInPropertyValue(String value) {
return value.replace("\0\0", ",");
}
+ /**
+ * creates the instance of the {@link
org.apache.commons.configuration2.PropertiesConfiguration}
+ * with custom od default {@link
org.apache.commons.configuration2.PropertiesConfiguration.IOFactory}
Review Comment:
typo: od?
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/SegmentMetadataPropertyConfigTest.java:
##########
@@ -0,0 +1,213 @@
+/**
+ * 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 SegmentMetadataPropertyConfigTest {
+ private static final File TEMP_DIR = new File(FileUtils.getTempDirectory(),
"SegmentMetadataPropertyConfigTest");
+ 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 {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataPropertyConfigurationWithHeader()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ // save the configuration.
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE,
+ PropertyIOFactoryKind.SegmentMetadataIOFactory.getVersion());
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
SegmentMetadataPropertyIOFactory.class);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataReaderWithSpecialCharsPropertyKeys()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, 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, 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, true);
+
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
SegmentMetadataPropertyIOFactory.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");
+ assertNotNull(tableName);
+ assertEquals(tableName, "fineFoodReviews");
+
+ // asserting table name property from segment metadata
+ String segmentName = configuration.getString("segment.name");
+ assertNotNull(tableName);
Review Comment:
duplicate?
##########
pinot-spi/src/test/java/org/apache/pinot/spi/env/SegmentMetadataPropertyConfigTest.java:
##########
@@ -0,0 +1,213 @@
+/**
+ * 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 SegmentMetadataPropertyConfigTest {
+ private static final File TEMP_DIR = new File(FileUtils.getTempDirectory(),
"SegmentMetadataPropertyConfigTest");
+ 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 {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataPropertyConfigurationWithHeader()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+
+ // save the configuration.
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE,
+ PropertyIOFactoryKind.SegmentMetadataIOFactory.getVersion());
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
SegmentMetadataPropertyIOFactory.class);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY);
+ }
+
+ @Test
+ public void testSegmentMetadataReaderWithSpecialCharsPropertyKeys()
+ throws ConfigurationException {
+ PropertiesConfiguration configuration =
+ CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE,
true, true);
+
+ // setting the random value of the test keys
+ for (String key: TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR) {
+ configuration.setProperty(key, RandomStringUtils.randomAscii(5));
+ }
+ // recovered keys from the configuration.
+ List<String> recoveredKeys =
CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, TEST_PROPERTY_KEY_WITH_SPECIAL_CHAR);
+
+ CommonsConfigurationUtils.saveSegmentMetadataToFile(configuration,
CONFIG_FILE, null); // save the configuration.
+
+ // reading the configuration from saved file.
+ configuration =
CommonsConfigurationUtils.getSegmentMetadataFromFile(CONFIG_FILE, true, true);
+ recoveredKeys = CommonsConfigurationUtils.getKeys(configuration);
+ testPropertyKeys(recoveredKeys, 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, 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, true);
+
+ // assert that Header is not null for the config.
+ assertNotNull(configuration.getHeader());
+
+ // assert that configuration has SegmentMetadataPropertyIOFactory
+ assertEquals(configuration.getIOFactory().getClass(),
SegmentMetadataPropertyIOFactory.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");
+ assertNotNull(tableName);
+ assertEquals(tableName, "fineFoodReviews");
+
+ // asserting table name property from segment metadata
+ String segmentName = configuration.getString("segment.name");
+ assertNotNull(tableName);
+ assertEquals(segmentName, "fineFoodReviews_OFFLINE_0");
+
+ // asserting segment dimension column names from segment metadata
+ String[] segmentDimensionColumnNames =
configuration.getStringArray("segment.dimension.column.names");
+ assertNotNull(segmentDimensionColumnNames);
+ 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");
+ assertNotNull(segmentIndexVersion);
Review Comment:
can remove
--
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
