Bill-hbrhbr commented on code in PR #13003:
URL: https://github.com/apache/pinot/pull/13003#discussion_r1583243217
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/TextIndexConfig.java:
##########
@@ -75,6 +82,15 @@ public TextIndexConfig(@JsonProperty("disabled") Boolean
disabled, @JsonProperty
luceneMaxBufferSizeMB == null ?
LUCENE_INDEX_DEFAULT_MAX_BUFFER_SIZE_MB : luceneMaxBufferSizeMB;
_luceneAnalyzerClass = (luceneAnalyzerClass == null ||
luceneAnalyzerClass.isEmpty())
? FieldConfig.TEXT_INDEX_DEFAULT_LUCENE_ANALYZER_CLASS :
luceneAnalyzerClass;
+
+ // Note that we cannot depend on jackson's default behavior to
automatically coerce the comma delimited args to
+ // List<String>. This is because the args may contain comma and other
special characters such as space. Therefore,
+ // we use our own csv parser to parse the values directly.
+ _luceneAnalyzerClassArgs = CsvParser.parse(luceneAnalyzerClassArgs, true,
false);
+ _luceneAnalyzerClassArgTypes =
+ luceneAnalyzerClassArgTypes == null ? Collections.emptyList() :
luceneAnalyzerClassArgTypes;
Review Comment:
Is it safer to use the CsvParser to trim the arg type string list passed in
by the user?
```suggestion
_luceneAnalyzerClassArgTypes =
CsvParser.parse(luceneAnalyzerClassArgTypes, false, true);
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/invertedindex/RealtimeLuceneTextIndex.java:
##########
@@ -181,6 +192,27 @@ private MutableRoaringBitmap getPinotDocIds(IndexSearcher
indexSearcher, Mutable
return actualDocIDs;
}
+ private Constructor<QueryParserBase>
getQueryParserWithStringAndAnalyzerTypeConstructor(String queryParserClassName)
+ throws ReflectiveOperationException {
+ // Fail-fast if the query parser is specified class is not QueryParseBase
class
+ final Class<?> queryParserClass = Class.forName(queryParserClassName);
+ if (!QueryParserBase.class.isAssignableFrom(queryParserClass)) {
+ throw new ReflectiveOperationException("The specified lucene query
parser class " + queryParserClassName
+ + " is not assignable from " + QueryParserBase.class.getName());
+ }
+ // Fail-fast if the query parser does not have the required constructor
used by this class
+ try {
+ queryParserClass.getConstructor(String.class, Analyzer.class);
+ } catch (NoSuchMethodException ex) {
+ throw new ReflectiveOperationException("The specified lucene query
parser class " + queryParserClassName
Review Comment:
Maybe we can use a subset of `ReflectiveOperationException` to make the
exception type more precise
```suggestion
throw new NoSuchMethodException("The specified lucene query parser
class " + queryParserClassName
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/store/SingleFileIndexDirectoryTest.java:
##########
@@ -235,7 +235,7 @@ public void testCleanupRemovedIndices()
public void testRemoveTextIndices()
throws IOException, ConfigurationException {
TextIndexConfig config =
- new TextIndexConfig(false, null, null, false, false, null, null,
true, 500, null, false);
+ new TextIndexConfig(false, null, null, false, false, null, null,
true, 500, null, null, null, null, false);
Review Comment:
Use textIndexConfigBuilder
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectoryTest.java:
##########
@@ -202,7 +202,7 @@ public void nativeTextIndexIsDeleted()
public void testRemoveTextIndices()
throws IOException {
TextIndexConfig config =
- new TextIndexConfig(false, null, null, false, false, null, null,
true, 500, null, false);
+ new TextIndexConfig(false, null, null, false, false, null, null,
true, 500, null, null, null, null, false);
Review Comment:
Use textIndexConfigBuilder
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/TextIndexUtils.java:
##########
@@ -108,10 +113,141 @@ private static List<String> parseEntryAsString(@Nullable
Map<String, String> col
.collect(Collectors.toList());
}
- public static Analyzer getAnalyzerFromClassName(String luceneAnalyzerClass)
- throws ReflectiveOperationException {
- // Support instantiation with default constructor for now unless customized
- return (Analyzer)
Class.forName(luceneAnalyzerClass).getConstructor().newInstance();
+ public static Analyzer getAnalyzer(TextIndexConfig config) throws
ReflectiveOperationException {
+ String luceneAnalyzerClassName = config.getLuceneAnalyzerClass();
+ List<String> luceneAnalyzerClassArgs = config.getLuceneAnalyzerClassArgs();
+ List<String> luceneAnalyzerClassArgsTypes =
config.getLuceneAnalyzerClassArgTypes();
+
+ if (null == luceneAnalyzerClassName || luceneAnalyzerClassName.isEmpty()
+ ||
(luceneAnalyzerClassName.equals(StandardAnalyzer.class.getName())
+ && luceneAnalyzerClassArgs.isEmpty() &&
luceneAnalyzerClassArgsTypes.isEmpty())) {
+ // When there is no analyzer defined, or when StandardAnalyzer (default)
is used without arguments,
+ // use existing logic to obtain an instance of StandardAnalyzer with
customized stop words
+ return TextIndexUtils.getStandardAnalyzerWithCustomizedStopWords(
+ config.getStopWordsInclude(), config.getStopWordsExclude());
+ } else {
+ // Custom analyzer + custom configs via reflection
+ if (luceneAnalyzerClassArgs.size() !=
luceneAnalyzerClassArgsTypes.size()) {
+ throw new ReflectiveOperationException("Mismatch of the number of
analyzer arguments and arguments types.");
+ }
+
+ // Generate args type list
+ List<Class<?>> argClasses = new ArrayList<>();
+ for (String argType : luceneAnalyzerClassArgsTypes) {
+ argClasses.add(parseSupportedTypes(argType));
+ }
+
+ // Best effort coercion to the analyzer argument type
+ // Note only a subset of class types is supported, unsupported ones can
be added in the future
+ List<Object> argValues = new ArrayList<>();
+ for (int i = 0; i < luceneAnalyzerClassArgs.size(); i++) {
+ argValues.add(parseSupportedTypeValues(luceneAnalyzerClassArgs.get(i),
argClasses.get(i)));
+ }
+
+ // Initialize the custom analyzer class with custom analyzer args
+ Class<?> luceneAnalyzerClass = Class.forName(luceneAnalyzerClassName);
+ Analyzer analyzer;
+ if (!Analyzer.class.isAssignableFrom(luceneAnalyzerClass)) {
+ String exceptionMessage = "Custom analyzer must be a child of " +
Analyzer.class.getCanonicalName();
+ LOGGER.error(exceptionMessage);
+ throw new ReflectiveOperationException(exceptionMessage);
+ }
+
+ // Return a new instance of custom lucene analyzer class
+ return (Analyzer)
luceneAnalyzerClass.getConstructor(argClasses.toArray(new Class<?>[0]))
+ .newInstance(argValues.toArray(new Object[0]));
+ }
+ }
+
+ /**
+ * Parse the Java value type specified in the type string
+ * @param valueTypeString FQCN of the value type class or the name of the
primitive value type
+ * @return Class object of the value type
+ * @throws ClassNotFoundException when the value type is not supported
+ */
+ public static Class<?> parseSupportedTypes(String valueTypeString) throws
ClassNotFoundException {
+ try {
+ // Support both primitive types + class
+ switch (valueTypeString) {
+ case "java.lang.Byte.TYPE":
+ return Byte.TYPE;
+ case "java.lang.Short.TYPE":
+ return Short.TYPE;
+ case "java.lang.Integer.TYPE":
+ return Integer.TYPE;
+ case "java.lang.Long.TYPE":
+ return Long.TYPE;
+ case "java.lang.Float.TYPE":
+ return Float.TYPE;
+ case "java.lang.Double.TYPE":
+ return Double.TYPE;
+ case "java.lang.Boolean.TYPE":
+ return Boolean.TYPE;
+ case "java.lang.Character.TYPE":
+ return Character.TYPE;
+ default:
+ return Class.forName(valueTypeString);
+ }
+ } catch (ClassNotFoundException ex) {
+ LOGGER.error("Analyzer argument class type not found: " +
valueTypeString);
+ throw ex;
+ }
+ }
+
+ /**
+ * Attempt to coerce string into supported value type
+ * @param stringValue string representation of the value
+ * @param clazz of the value
+ * @return class object of the value, auto-boxed if it is a primitive type
+ * @throws ReflectiveOperationException if value cannot be coerced without
ambiguity or encountered unsupported type
+ */
+ public static Object parseSupportedTypeValues(String stringValue, Class<?>
clazz)
+ throws ReflectiveOperationException {
+ try {
+ if (clazz.equals(String.class)) {
+ return stringValue;
+ } else if (clazz.equals(Byte.class) || clazz.equals(Byte.TYPE)) {
+ return Byte.parseByte(stringValue);
+ } else if (clazz.equals(Short.class) || clazz.equals(Short.TYPE)) {
+ return Short.parseShort(stringValue);
+ } else if (clazz.equals(Integer.class) || clazz.equals(Integer.TYPE)) {
+ return Integer.parseInt(stringValue);
+ } else if (clazz.equals(Long.class) || clazz.equals(Long.TYPE)) {
+ return Long.parseLong(stringValue);
+ } else if (clazz.equals(Float.class) || clazz.equals(Float.TYPE)) {
+ return Float.parseFloat(stringValue);
+ } else if (clazz.equals(Double.class) || clazz.equals(Double.TYPE)) {
+ return Double.parseDouble(stringValue);
+ } else if (clazz.equals(Boolean.class) || clazz.equals(Boolean.TYPE)) {
+ // Note we cannot use Boolean.parseBoolean here because it treats
"abc" as false which
+ // introduces unexpected parsing results. We should validate the input
by accepting only
+ // true|false in a case-insensitive manner, for all other values,
return an exception.
+ String lowerCaseStringValue = stringValue.toLowerCase();
+ if (lowerCaseStringValue.equals("true")) {
+ return true;
+ } else if (lowerCaseStringValue.equals("false")) {
+ return false;
+ }
+ throw new ReflectiveOperationException();
+ } else if (clazz.equals(Character.class) ||
clazz.equals(Character.TYPE)) {
+ if (stringValue.length() == 1) {
+ return stringValue.charAt(0);
+ }
+ throw new ReflectiveOperationException();
+ } else {
+ throw new UnsupportedOperationException();
+ }
+ } catch (NumberFormatException | ReflectiveOperationException ex) {
Review Comment:
`IllegalArgumentException` contains `NumberFormatException`, which is
convenient here
```suggestion
} catch (IllegalArgumentException ex) {
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/TextIndexUtils.java:
##########
@@ -108,10 +113,141 @@ private static List<String> parseEntryAsString(@Nullable
Map<String, String> col
.collect(Collectors.toList());
}
- public static Analyzer getAnalyzerFromClassName(String luceneAnalyzerClass)
- throws ReflectiveOperationException {
- // Support instantiation with default constructor for now unless customized
- return (Analyzer)
Class.forName(luceneAnalyzerClass).getConstructor().newInstance();
+ public static Analyzer getAnalyzer(TextIndexConfig config) throws
ReflectiveOperationException {
+ String luceneAnalyzerClassName = config.getLuceneAnalyzerClass();
+ List<String> luceneAnalyzerClassArgs = config.getLuceneAnalyzerClassArgs();
+ List<String> luceneAnalyzerClassArgsTypes =
config.getLuceneAnalyzerClassArgTypes();
+
+ if (null == luceneAnalyzerClassName || luceneAnalyzerClassName.isEmpty()
+ ||
(luceneAnalyzerClassName.equals(StandardAnalyzer.class.getName())
+ && luceneAnalyzerClassArgs.isEmpty() &&
luceneAnalyzerClassArgsTypes.isEmpty())) {
+ // When there is no analyzer defined, or when StandardAnalyzer (default)
is used without arguments,
+ // use existing logic to obtain an instance of StandardAnalyzer with
customized stop words
+ return TextIndexUtils.getStandardAnalyzerWithCustomizedStopWords(
+ config.getStopWordsInclude(), config.getStopWordsExclude());
+ } else {
+ // Custom analyzer + custom configs via reflection
+ if (luceneAnalyzerClassArgs.size() !=
luceneAnalyzerClassArgsTypes.size()) {
+ throw new ReflectiveOperationException("Mismatch of the number of
analyzer arguments and arguments types.");
+ }
+
+ // Generate args type list
+ List<Class<?>> argClasses = new ArrayList<>();
+ for (String argType : luceneAnalyzerClassArgsTypes) {
+ argClasses.add(parseSupportedTypes(argType));
+ }
+
+ // Best effort coercion to the analyzer argument type
+ // Note only a subset of class types is supported, unsupported ones can
be added in the future
+ List<Object> argValues = new ArrayList<>();
+ for (int i = 0; i < luceneAnalyzerClassArgs.size(); i++) {
+ argValues.add(parseSupportedTypeValues(luceneAnalyzerClassArgs.get(i),
argClasses.get(i)));
+ }
+
+ // Initialize the custom analyzer class with custom analyzer args
+ Class<?> luceneAnalyzerClass = Class.forName(luceneAnalyzerClassName);
+ Analyzer analyzer;
+ if (!Analyzer.class.isAssignableFrom(luceneAnalyzerClass)) {
+ String exceptionMessage = "Custom analyzer must be a child of " +
Analyzer.class.getCanonicalName();
+ LOGGER.error(exceptionMessage);
+ throw new ReflectiveOperationException(exceptionMessage);
+ }
+
+ // Return a new instance of custom lucene analyzer class
+ return (Analyzer)
luceneAnalyzerClass.getConstructor(argClasses.toArray(new Class<?>[0]))
+ .newInstance(argValues.toArray(new Object[0]));
+ }
+ }
+
+ /**
+ * Parse the Java value type specified in the type string
+ * @param valueTypeString FQCN of the value type class or the name of the
primitive value type
+ * @return Class object of the value type
+ * @throws ClassNotFoundException when the value type is not supported
+ */
+ public static Class<?> parseSupportedTypes(String valueTypeString) throws
ClassNotFoundException {
+ try {
+ // Support both primitive types + class
+ switch (valueTypeString) {
+ case "java.lang.Byte.TYPE":
+ return Byte.TYPE;
+ case "java.lang.Short.TYPE":
+ return Short.TYPE;
+ case "java.lang.Integer.TYPE":
+ return Integer.TYPE;
+ case "java.lang.Long.TYPE":
+ return Long.TYPE;
+ case "java.lang.Float.TYPE":
+ return Float.TYPE;
+ case "java.lang.Double.TYPE":
+ return Double.TYPE;
+ case "java.lang.Boolean.TYPE":
+ return Boolean.TYPE;
+ case "java.lang.Character.TYPE":
+ return Character.TYPE;
+ default:
+ return Class.forName(valueTypeString);
+ }
+ } catch (ClassNotFoundException ex) {
+ LOGGER.error("Analyzer argument class type not found: " +
valueTypeString);
+ throw ex;
+ }
+ }
+
+ /**
+ * Attempt to coerce string into supported value type
+ * @param stringValue string representation of the value
+ * @param clazz of the value
+ * @return class object of the value, auto-boxed if it is a primitive type
+ * @throws ReflectiveOperationException if value cannot be coerced without
ambiguity or encountered unsupported type
+ */
+ public static Object parseSupportedTypeValues(String stringValue, Class<?>
clazz)
+ throws ReflectiveOperationException {
+ try {
+ if (clazz.equals(String.class)) {
+ return stringValue;
+ } else if (clazz.equals(Byte.class) || clazz.equals(Byte.TYPE)) {
+ return Byte.parseByte(stringValue);
+ } else if (clazz.equals(Short.class) || clazz.equals(Short.TYPE)) {
+ return Short.parseShort(stringValue);
+ } else if (clazz.equals(Integer.class) || clazz.equals(Integer.TYPE)) {
+ return Integer.parseInt(stringValue);
+ } else if (clazz.equals(Long.class) || clazz.equals(Long.TYPE)) {
+ return Long.parseLong(stringValue);
+ } else if (clazz.equals(Float.class) || clazz.equals(Float.TYPE)) {
+ return Float.parseFloat(stringValue);
+ } else if (clazz.equals(Double.class) || clazz.equals(Double.TYPE)) {
+ return Double.parseDouble(stringValue);
+ } else if (clazz.equals(Boolean.class) || clazz.equals(Boolean.TYPE)) {
+ // Note we cannot use Boolean.parseBoolean here because it treats
"abc" as false which
+ // introduces unexpected parsing results. We should validate the input
by accepting only
+ // true|false in a case-insensitive manner, for all other values,
return an exception.
+ String lowerCaseStringValue = stringValue.toLowerCase();
+ if (lowerCaseStringValue.equals("true")) {
+ return true;
+ } else if (lowerCaseStringValue.equals("false")) {
+ return false;
+ }
+ throw new ReflectiveOperationException();
Review Comment:
A more appropriate exception class
```suggestion
throw new IllegalArgumentException();
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/converter/RealtimeSegmentConverterTest.java:
##########
@@ -474,7 +474,7 @@ public void testSegmentBuilderWithReuse(boolean
columnMajorSegmentBuilder, Strin
IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
TextIndexConfig textIndexConfig =
new TextIndexConfig(false, null, null, false, false,
Collections.emptyList(), Collections.emptyList(), false,
Review Comment:
This allows us to not have to update unit tests each time we add a new
config key to the text index config. Also makes it clearer which configs are
different from default values.
```suggestion
TextIndexConfig textIndexConfig = new
TextIndexConfigBuilder().withUseANDForMultiTermQueries(false).build();
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/text/TextIndexConfigBuilder.java:
##########
@@ -23,6 +23,7 @@
import javax.annotation.Nullable;
import org.apache.pinot.segment.local.segment.store.TextIndexUtils;
import org.apache.pinot.segment.spi.index.TextIndexConfig;
+import org.apache.pinot.segment.spi.utils.CsvParser;
import org.apache.pinot.spi.config.table.FSTType;
import org.apache.pinot.spi.config.table.FieldConfig;
Review Comment:
Add a default constructor that doesn't take in any argument(s)
```
public TextIndexConfigBuilder() {
super((FSTType) null);
}
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/impl/invertedindex/NativeAndLuceneMutableTextIndexTest.java:
##########
@@ -72,7 +72,7 @@ private String[][] getMVTextData() {
public void setUp()
throws Exception {
TextIndexConfig config =
- new TextIndexConfig(false, null, null, false, false, null, null, true,
500, null, false);
+ new TextIndexConfig(false, null, null, false, false, null, null, true,
500, null, null, null, null, false);
Review Comment:
Change to use TextIndexConfigBuilder
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/TextIndexConfig.java:
##########
@@ -168,12 +210,16 @@ public AbstractBuilder(TextIndexConfig other) {
_luceneUseCompoundFile = other._luceneUseCompoundFile;
_luceneMaxBufferSizeMB = other._luceneMaxBufferSizeMB;
_luceneAnalyzerClass = other._luceneAnalyzerClass;
+ _luceneAnalyzerClassArgs = other._luceneAnalyzerClassArgs;
+ _luceneAnalyzerClassArgTypes = other._luceneAnalyzerClassArgTypes;
+ _luceneQueryParserClass = other._luceneQueryParserClass;
_enablePrefixSuffixMatchingInPhraseQueries =
other._enablePrefixSuffixMatchingInPhraseQueries;
}
public TextIndexConfig build() {
return new TextIndexConfig(false, _fstType, _rawValueForTextIndex,
_enableQueryCache, _useANDForMultiTermQueries,
_stopWordsInclude, _stopWordsExclude, _luceneUseCompoundFile,
_luceneMaxBufferSizeMB, _luceneAnalyzerClass,
+ String.join(",", _luceneAnalyzerClassArgs),
_luceneAnalyzerClassArgTypes, _luceneQueryParserClass,
_enablePrefixSuffixMatchingInPhraseQueries);
}
Review Comment:
Add `withEnableQueryCache` and `withUseANDForMultiTermQueries`? Makes unit
testing easier
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/utils/CsvParser.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.spi.utils;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import javax.annotation.Nullable;
+
+public class CsvParser {
+ private CsvParser() {
+ // Hide utility class default constructor
+ }
+
+ /**
+ * Parse the input csv string with customizable parsing behavior.
Sometimes the individual values may contain comma
+ * and other white space characters. These characters are sometimes
expected to be part of the actual argument.
+ *
+ * @param input string to split on comma
+ * @param escapeComma whether we should ignore "\," during splitting,
replace it with "," after split
Review Comment:
```suggestion
* @param escapeComma if true, we don't split on escaped commas, and we
replace "\," with "," after the split
```
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/TextIndexConfig.java:
##########
@@ -168,12 +210,16 @@ public AbstractBuilder(TextIndexConfig other) {
_luceneUseCompoundFile = other._luceneUseCompoundFile;
_luceneMaxBufferSizeMB = other._luceneMaxBufferSizeMB;
_luceneAnalyzerClass = other._luceneAnalyzerClass;
+ _luceneAnalyzerClassArgs = other._luceneAnalyzerClassArgs;
+ _luceneAnalyzerClassArgTypes = other._luceneAnalyzerClassArgTypes;
+ _luceneQueryParserClass = other._luceneQueryParserClass;
_enablePrefixSuffixMatchingInPhraseQueries =
other._enablePrefixSuffixMatchingInPhraseQueries;
}
public TextIndexConfig build() {
return new TextIndexConfig(false, _fstType, _rawValueForTextIndex,
_enableQueryCache, _useANDForMultiTermQueries,
_stopWordsInclude, _stopWordsExclude, _luceneUseCompoundFile,
_luceneMaxBufferSizeMB, _luceneAnalyzerClass,
+ String.join(",", _luceneAnalyzerClassArgs),
_luceneAnalyzerClassArgTypes, _luceneQueryParserClass,
Review Comment:
Although it may seem unnecessary here, but we should rejoin the class arg
types as string as well since the string passed in from user side (text index
config) may contain whitespaces that need to be trimmed
```suggestion
String.join(",", _luceneAnalyzerClassArgs), String.join(",",
_luceneAnalyzerClassArgTypes), _luceneQueryParserClass,
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java:
##########
@@ -54,6 +54,11 @@ public class FieldConfig extends BaseJsonConfig {
public static final String TEXT_INDEX_LUCENE_ANALYZER_CLASS =
"luceneAnalyzerClass";
public static final String TEXT_INDEX_DEFAULT_LUCENE_ANALYZER_CLASS =
"org.apache.lucene.analysis.standard.StandardAnalyzer";
+ public static final String TEXT_INDEX_LUCENE_ANALYZER_CLASS_ARGS =
"luceneAnalyzerClassArgs";
+ public static final String TEXT_INDEX_LUCENE_ANALYZER_CLASS_ARG_TYPES =
"luceneAnalyzerClassArgTypes";
+ public static final String TEXT_INDEX_LUCENE_QUERY_PARSER_CLASS =
"luceneQueryParserClass";
+ public static final String TEXT_INDEX_DEFAULT_LUCENE_QUERY_PARSER_CLASS =
+ "org.apache.lucene.queryparser.classic.QueryParser";
Review Comment:
Move the config keys to the top and group the 2 default values together.
```suggestion
public static final String TEXT_INDEX_LUCENE_ANALYZER_CLASS_ARGS =
"luceneAnalyzerClassArgs";
public static final String TEXT_INDEX_LUCENE_ANALYZER_CLASS_ARG_TYPES =
"luceneAnalyzerClassArgTypes";
public static final String TEXT_INDEX_LUCENE_QUERY_PARSER_CLASS =
"luceneQueryParserClass";
public static final String TEXT_INDEX_DEFAULT_LUCENE_ANALYZER_CLASS =
"org.apache.lucene.analysis.standard.StandardAnalyzer";
public static final String TEXT_INDEX_DEFAULT_LUCENE_QUERY_PARSER_CLASS =
"org.apache.lucene.queryparser.classic.QueryParser";
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectoryTest.java:
##########
@@ -264,7 +264,7 @@ public void testRemoveTextIndices()
public void testGetColumnIndices()
throws IOException {
TextIndexConfig config =
- new TextIndexConfig(false, null, null, false, false, null, null,
true, 500, null, false);
+ new TextIndexConfig(false, null, null, false, false, null, null,
true, 500, null, null, null, null, false);
Review Comment:
Use textIndexConfigBuilder
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/TextIndexConfig.java:
##########
@@ -61,6 +65,9 @@ public TextIndexConfig(@JsonProperty("disabled") Boolean
disabled, @JsonProperty
@JsonProperty("luceneUseCompoundFile") Boolean luceneUseCompoundFile,
@JsonProperty("luceneMaxBufferSizeMB") Integer luceneMaxBufferSizeMB,
@JsonProperty("luceneAnalyzerClass") String luceneAnalyzerClass,
+ @JsonProperty("luceneAnalyzerClassArgs") String luceneAnalyzerClassArgs,
+ @JsonProperty("luceneAnalyzerClassArgTypes") List<String>
luceneAnalyzerClassArgTypes,
Review Comment:
Is it safer to use the CsvParser to trim the arg type string list passed in
by the user?
```suggestion
@JsonProperty("luceneAnalyzerClassArgTypes") String
luceneAnalyzerClassArgTypes,
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/TextIndexUtils.java:
##########
@@ -108,10 +113,141 @@ private static List<String> parseEntryAsString(@Nullable
Map<String, String> col
.collect(Collectors.toList());
}
- public static Analyzer getAnalyzerFromClassName(String luceneAnalyzerClass)
- throws ReflectiveOperationException {
- // Support instantiation with default constructor for now unless customized
- return (Analyzer)
Class.forName(luceneAnalyzerClass).getConstructor().newInstance();
+ public static Analyzer getAnalyzer(TextIndexConfig config) throws
ReflectiveOperationException {
+ String luceneAnalyzerClassName = config.getLuceneAnalyzerClass();
+ List<String> luceneAnalyzerClassArgs = config.getLuceneAnalyzerClassArgs();
+ List<String> luceneAnalyzerClassArgsTypes =
config.getLuceneAnalyzerClassArgTypes();
+
+ if (null == luceneAnalyzerClassName || luceneAnalyzerClassName.isEmpty()
+ ||
(luceneAnalyzerClassName.equals(StandardAnalyzer.class.getName())
+ && luceneAnalyzerClassArgs.isEmpty() &&
luceneAnalyzerClassArgsTypes.isEmpty())) {
+ // When there is no analyzer defined, or when StandardAnalyzer (default)
is used without arguments,
+ // use existing logic to obtain an instance of StandardAnalyzer with
customized stop words
+ return TextIndexUtils.getStandardAnalyzerWithCustomizedStopWords(
+ config.getStopWordsInclude(), config.getStopWordsExclude());
+ } else {
+ // Custom analyzer + custom configs via reflection
+ if (luceneAnalyzerClassArgs.size() !=
luceneAnalyzerClassArgsTypes.size()) {
+ throw new ReflectiveOperationException("Mismatch of the number of
analyzer arguments and arguments types.");
+ }
+
+ // Generate args type list
+ List<Class<?>> argClasses = new ArrayList<>();
+ for (String argType : luceneAnalyzerClassArgsTypes) {
+ argClasses.add(parseSupportedTypes(argType));
+ }
+
+ // Best effort coercion to the analyzer argument type
+ // Note only a subset of class types is supported, unsupported ones can
be added in the future
+ List<Object> argValues = new ArrayList<>();
+ for (int i = 0; i < luceneAnalyzerClassArgs.size(); i++) {
+ argValues.add(parseSupportedTypeValues(luceneAnalyzerClassArgs.get(i),
argClasses.get(i)));
+ }
+
+ // Initialize the custom analyzer class with custom analyzer args
+ Class<?> luceneAnalyzerClass = Class.forName(luceneAnalyzerClassName);
+ Analyzer analyzer;
+ if (!Analyzer.class.isAssignableFrom(luceneAnalyzerClass)) {
+ String exceptionMessage = "Custom analyzer must be a child of " +
Analyzer.class.getCanonicalName();
+ LOGGER.error(exceptionMessage);
+ throw new ReflectiveOperationException(exceptionMessage);
+ }
+
+ // Return a new instance of custom lucene analyzer class
+ return (Analyzer)
luceneAnalyzerClass.getConstructor(argClasses.toArray(new Class<?>[0]))
+ .newInstance(argValues.toArray(new Object[0]));
+ }
+ }
+
+ /**
+ * Parse the Java value type specified in the type string
+ * @param valueTypeString FQCN of the value type class or the name of the
primitive value type
+ * @return Class object of the value type
+ * @throws ClassNotFoundException when the value type is not supported
+ */
+ public static Class<?> parseSupportedTypes(String valueTypeString) throws
ClassNotFoundException {
+ try {
+ // Support both primitive types + class
+ switch (valueTypeString) {
+ case "java.lang.Byte.TYPE":
+ return Byte.TYPE;
+ case "java.lang.Short.TYPE":
+ return Short.TYPE;
+ case "java.lang.Integer.TYPE":
+ return Integer.TYPE;
+ case "java.lang.Long.TYPE":
+ return Long.TYPE;
+ case "java.lang.Float.TYPE":
+ return Float.TYPE;
+ case "java.lang.Double.TYPE":
+ return Double.TYPE;
+ case "java.lang.Boolean.TYPE":
+ return Boolean.TYPE;
+ case "java.lang.Character.TYPE":
+ return Character.TYPE;
+ default:
+ return Class.forName(valueTypeString);
+ }
+ } catch (ClassNotFoundException ex) {
+ LOGGER.error("Analyzer argument class type not found: " +
valueTypeString);
+ throw ex;
+ }
+ }
+
+ /**
+ * Attempt to coerce string into supported value type
+ * @param stringValue string representation of the value
+ * @param clazz of the value
+ * @return class object of the value, auto-boxed if it is a primitive type
+ * @throws ReflectiveOperationException if value cannot be coerced without
ambiguity or encountered unsupported type
+ */
+ public static Object parseSupportedTypeValues(String stringValue, Class<?>
clazz)
+ throws ReflectiveOperationException {
+ try {
+ if (clazz.equals(String.class)) {
+ return stringValue;
+ } else if (clazz.equals(Byte.class) || clazz.equals(Byte.TYPE)) {
+ return Byte.parseByte(stringValue);
+ } else if (clazz.equals(Short.class) || clazz.equals(Short.TYPE)) {
+ return Short.parseShort(stringValue);
+ } else if (clazz.equals(Integer.class) || clazz.equals(Integer.TYPE)) {
+ return Integer.parseInt(stringValue);
+ } else if (clazz.equals(Long.class) || clazz.equals(Long.TYPE)) {
+ return Long.parseLong(stringValue);
+ } else if (clazz.equals(Float.class) || clazz.equals(Float.TYPE)) {
+ return Float.parseFloat(stringValue);
+ } else if (clazz.equals(Double.class) || clazz.equals(Double.TYPE)) {
+ return Double.parseDouble(stringValue);
+ } else if (clazz.equals(Boolean.class) || clazz.equals(Boolean.TYPE)) {
+ // Note we cannot use Boolean.parseBoolean here because it treats
"abc" as false which
+ // introduces unexpected parsing results. We should validate the input
by accepting only
+ // true|false in a case-insensitive manner, for all other values,
return an exception.
+ String lowerCaseStringValue = stringValue.toLowerCase();
+ if (lowerCaseStringValue.equals("true")) {
+ return true;
+ } else if (lowerCaseStringValue.equals("false")) {
+ return false;
+ }
+ throw new ReflectiveOperationException();
+ } else if (clazz.equals(Character.class) ||
clazz.equals(Character.TYPE)) {
+ if (stringValue.length() == 1) {
+ return stringValue.charAt(0);
+ }
+ throw new ReflectiveOperationException();
Review Comment:
```suggestion
throw new IllegalArgumentException();
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/TextIndexUtils.java:
##########
@@ -108,10 +113,141 @@ private static List<String> parseEntryAsString(@Nullable
Map<String, String> col
.collect(Collectors.toList());
}
- public static Analyzer getAnalyzerFromClassName(String luceneAnalyzerClass)
- throws ReflectiveOperationException {
- // Support instantiation with default constructor for now unless customized
- return (Analyzer)
Class.forName(luceneAnalyzerClass).getConstructor().newInstance();
+ public static Analyzer getAnalyzer(TextIndexConfig config) throws
ReflectiveOperationException {
+ String luceneAnalyzerClassName = config.getLuceneAnalyzerClass();
+ List<String> luceneAnalyzerClassArgs = config.getLuceneAnalyzerClassArgs();
+ List<String> luceneAnalyzerClassArgsTypes =
config.getLuceneAnalyzerClassArgTypes();
+
+ if (null == luceneAnalyzerClassName || luceneAnalyzerClassName.isEmpty()
+ ||
(luceneAnalyzerClassName.equals(StandardAnalyzer.class.getName())
+ && luceneAnalyzerClassArgs.isEmpty() &&
luceneAnalyzerClassArgsTypes.isEmpty())) {
+ // When there is no analyzer defined, or when StandardAnalyzer (default)
is used without arguments,
+ // use existing logic to obtain an instance of StandardAnalyzer with
customized stop words
+ return TextIndexUtils.getStandardAnalyzerWithCustomizedStopWords(
+ config.getStopWordsInclude(), config.getStopWordsExclude());
+ } else {
+ // Custom analyzer + custom configs via reflection
+ if (luceneAnalyzerClassArgs.size() !=
luceneAnalyzerClassArgsTypes.size()) {
+ throw new ReflectiveOperationException("Mismatch of the number of
analyzer arguments and arguments types.");
+ }
+
+ // Generate args type list
+ List<Class<?>> argClasses = new ArrayList<>();
+ for (String argType : luceneAnalyzerClassArgsTypes) {
+ argClasses.add(parseSupportedTypes(argType));
+ }
+
+ // Best effort coercion to the analyzer argument type
+ // Note only a subset of class types is supported, unsupported ones can
be added in the future
+ List<Object> argValues = new ArrayList<>();
+ for (int i = 0; i < luceneAnalyzerClassArgs.size(); i++) {
+ argValues.add(parseSupportedTypeValues(luceneAnalyzerClassArgs.get(i),
argClasses.get(i)));
+ }
+
+ // Initialize the custom analyzer class with custom analyzer args
+ Class<?> luceneAnalyzerClass = Class.forName(luceneAnalyzerClassName);
+ Analyzer analyzer;
Review Comment:
Unused
```suggestion
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/realtime/impl/invertedindex/LuceneMutableTextIndexTest.java:
##########
@@ -55,13 +163,15 @@ private String[][] getRepeatedData() {
};
}
- @BeforeClass
- public void setUp()
- throws Exception {
- TextIndexConfig config =
- new TextIndexConfig(false, null, null, false, false, null, null,
true, 500, null, false);
- _realtimeLuceneTextIndex =
- new RealtimeLuceneTextIndex(TEXT_COLUMN_NAME, INDEX_DIR, "fooBar",
config);
+ private void configureIndex(String analyzerClass, String analyzerClassArgs,
List<String> analyzerClassArgTypes,
+ String queryParserClass) {
+ TextIndexConfig config = new TextIndexConfig(false, null, null, false,
false, null, null, true, 500,
+ analyzerClass, analyzerClassArgs, analyzerClassArgTypes,
queryParserClass, false);
Review Comment:
Use config builder
```suggestion
TextIndexConfigBuilder builder = new TextIndexConfigBuilder();
if (null != analyzerClass) {
builder.withLuceneAnalyzerClass(analyzerClass);
}
if (null != analyzerClassArgs) {
builder.withLuceneAnalyzerClassArgs(analyzerClassArgs);
}
if (null != analyzerClassArgTypes) {
builder.withLuceneAnalyzerClassArgTypes(analyzerClassArgTypes);
}
if (null != queryParserClass) {
builder.withLuceneQueryParserClass(queryParserClass);
}
TextIndexConfig config =
builder.withUseANDForMultiTermQueries(false).build();
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/store/SingleFileIndexDirectoryTest.java:
##########
@@ -341,7 +341,7 @@ public void testPersistIndexMaps() {
public void testGetColumnIndices()
throws Exception {
TextIndexConfig config =
- new TextIndexConfig(false, null, null, false, false, null, null,
true, 500, null, false);
+ new TextIndexConfig(false, null, null, false, false, null, null,
true, 500, null, null, null, null, false);
Review Comment:
Use textIndexConfigBuilder
--
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
