[GitHub] [pinot] walterddr commented on a diff in pull request #11048: [multistage] Register Pinot Transform Functions into Calcite

["walterddr (via GitHub)" <gi...@apache.org>]

walterddr commented on code in PR #11048:
URL: https://github.com/apache/pinot/pull/11048#discussion_r1257379055


##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -76,8 +89,13 @@ public enum TransformFunctionType {
   CAST("cast"),
 
   // string functions
-  JSONEXTRACTSCALAR("jsonExtractScalar"),
-  JSONEXTRACTKEY("jsonExtractKey"),
+  JSONEXTRACTSCALAR("jsonExtractScalar", ReturnTypes.cascade(
+      opBinding -> inferJsonExtractScalarExplicitTypeSpec(opBinding).orElse(
+          opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR, 2000)), SqlTypeTransforms.FORCE_NULLABLE),

Review Comment:
   we dont need to limit varchar to 2000 here



##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -187,7 +187,7 @@ public static Object jsonExtractScalar(String jsonFieldName, String jsonPath, St
     }
 
     @ScalarFunction(names = {"jsonExtractKey", "json_extract_key"}, isPlaceholder = true)
-    public static String jsonExtractKey(String jsonFieldName, String jsonPath) {
+    public static Object jsonExtractKey(String jsonFieldName, String jsonPath) {

Review Comment:
   JsonExtractKey always returns String right? a JSON key cannot be any other type?



##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -180,4 +210,46 @@ public String getName() {
   public List<String> getAliases() {
     return _aliases;
   }
+
+  public SqlReturnTypeInference getSqlReturnTypeInference() {
+    return _sqlReturnTypeInference;
+  }
+
+  public SqlOperandTypeChecker getSqlOperandTypeChecker() {
+    return _sqlOperandTypeChecker;
+  }
+
+  /** Returns the optional explicit returning type specification. */
+  private static Optional<RelDataType> inferJsonExtractScalarExplicitTypeSpec(SqlOperatorBinding opBinding) {
+    if (opBinding.getOperandCount() > 2
+        && opBinding.isOperandLiteral(2, false)) {
+      String operandType = opBinding.getOperandLiteralValue(2, String.class).toUpperCase();
+      return Optional.of(inferExplicitTypeSpec(operandType, opBinding.getTypeFactory()));
+    }
+    return Optional.empty();
+  }
+
+  private static RelDataType inferExplicitTypeSpec(String operandType, RelDataTypeFactory typeFactory) {
+    switch (operandType) {
+      case "INT":
+      case "INTEGER":

Review Comment:
   INTEGER is valid SQL type name



##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -180,4 +210,46 @@ public String getName() {
   public List<String> getAliases() {
     return _aliases;
   }
+
+  public SqlReturnTypeInference getSqlReturnTypeInference() {
+    return _sqlReturnTypeInference;
+  }
+
+  public SqlOperandTypeChecker getSqlOperandTypeChecker() {
+    return _sqlOperandTypeChecker;
+  }
+
+  /** Returns the optional explicit returning type specification. */
+  private static Optional<RelDataType> inferJsonExtractScalarExplicitTypeSpec(SqlOperatorBinding opBinding) {
+    if (opBinding.getOperandCount() > 2
+        && opBinding.isOperandLiteral(2, false)) {
+      String operandType = opBinding.getOperandLiteralValue(2, String.class).toUpperCase();
+      return Optional.of(inferExplicitTypeSpec(operandType, opBinding.getTypeFactory()));
+    }
+    return Optional.empty();
+  }
+
+  private static RelDataType inferExplicitTypeSpec(String operandType, RelDataTypeFactory typeFactory) {

Review Comment:
   ```suggestion
     private static RelDataType inferExplicitTypeSpec(String operandTypeStr, RelDataTypeFactory typeFactory) {
   ```



##########
pinot-query-planner/src/main/java/org/apache/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -248,4 +316,16 @@ private boolean isPinotAggregationFunction(String name) {
     }
     return aggFunctionType != null && !aggFunctionType.isNativeCalciteAggregationFunctionType();
   }
+
+  private boolean isPinotTransformFunction(String name) {
+    TransformFunctionType transformFunctionType = null;
+    if (isTransformFunctionRegisteredWithOperatorTable(name)) {
+      try {
+        transformFunctionType = TransformFunctionType.valueOf(name);
+      } catch (IllegalArgumentException e) {
+        // Ignore

Review Comment:
   this error shouldn't occur if the isTransform... method is correctly capture the name



##########
pinot-query-planner/src/main/java/org/apache/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -207,6 +222,49 @@ public final void initNoDuplicate() {
     }
   }
 
+  /**
+   * This registers transform functions defined in {@link org.apache.pinot.common.function.TransformFunctionType}
+   * which are multistage enabled.
+   */
+  private void initTransformFunctions() {

Review Comment:
   see my change in https://github.com/apache/pinot/pull/11068 
   this method can be greatly simplified



##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -298,24 +298,32 @@ public boolean isSuperTypeOf(ColumnDataType subTypeCandidate) {
     public DataType toDataType() {
       switch (this) {
         case INT:
+        case INT_ARRAY:

Review Comment:
   why this change? let's dont mix array support in here if we can make that separate



-- 
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