jackluo923 commented on code in PR #13003:
URL: https://github.com/apache/pinot/pull/13003#discussion_r1584153480
##########
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:
IllegalArgumentException is an unchecked exception which may not be ideal if
not caught. Here, it makes more sense to throw ReflectiveOperationException
because we are indeed performing reflective operation on the passed in value,
but ReflectiveOperationException is caught and handled by the current class.
##########
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 is an unchecked exception which may not be ideal if
not caught. Here, it makes more sense to throw ReflectiveOperationException
because we are indeed performing reflective operation on the passed in value,
but ReflectiveOperationException is caught and handled by the current class.
--
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
