[GitHub] [pinot] amrishlal commented on a change in pull request #8253: Fix string comparisons.

[GitBox <gi...@apache.org>]

amrishlal commented on a change in pull request #8253:
URL: https://github.com/apache/pinot/pull/8253#discussion_r816127861



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/statement/StringPredicateFilterOptimizer.java
##########
@@ -99,21 +104,37 @@ private static void replaceMinusWithCompareForStrings(Expression expression, Sch
     Function function = expression.getFunctionCall();
     String operator = function.getOperator();
     List<Expression> operands = function.getOperands();
-    if (operator.equals(MINUS_OPERATOR_NAME) && operands.size() == 2 && isStringColumn(operands.get(0), schema)
-        && isStringColumn(operands.get(1), schema)) {
+    if (operator.equals(MINUS_OPERATOR_NAME) && operands.size() == 2 && isString(operands.get(0), schema)
+        && isString(operands.get(1), schema)) {
       function.setOperator(STRCMP_OPERATOR_NAME);
     }
   }
 
-  /** @return true if expression is a column of string type. */
-  private static boolean isStringColumn(Expression expression, Schema schema) {
-    if (expression.getType() != ExpressionType.IDENTIFIER) {
-      // Expression is not a column.
-      return false;
+  /** @return true if expression is STRING column or a function that outputs STRING. */
+  private static boolean isString(Expression expression, Schema schema) {
+    ExpressionType expressionType = expression.getType();
+
+    if (expressionType == ExpressionType.IDENTIFIER) {
+      // Check if this is a STRING column.
+      String column = expression.getIdentifier().getName();
+      FieldSpec fieldSpec = schema.getFieldSpecFor(column);
+      return fieldSpec != null && fieldSpec.getDataType() == FieldSpec.DataType.STRING;
     }
 
-    String column = expression.getIdentifier().getName();
-    FieldSpec fieldSpec = schema.getFieldSpecFor(column);
-    return fieldSpec != null && fieldSpec.getDataType() == FieldSpec.DataType.STRING;
+    // Check if the function returns STRING as output.
+    return (expressionType == ExpressionType.FUNCTION && STRING_OUTPUT_FUNCTIONS
+        .contains(expression.getFunctionCall().getOperator().toUpperCase()));
+  }
+
+  /** List of string functions that return STRING output. */
+  private static Set<String> getStringOutputFunctionList() {
+    Set<String> set = new HashSet<>();
+    Method[] methods = StringFunctions.class.getDeclaredMethods();
+    for (Method method : methods) {
+      if (method.getReturnType() == String.class) {
+        set.add(method.getName().toUpperCase());
+      }
+    }
+    return set;

Review comment:
       Wouldn't using @ScalarFunction return names of all scalar functions and how would the output type of the function be resolved? We just want String scalar functions and that too only the once that return a string as output. All the string scalar functions are declared in StringFunctions and hence this code.




-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org