[GitHub] [druid] suneet-s commented on a change in pull request #10370: add computed Expr output types

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

suneet-s commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r486048490



##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -74,6 +75,9 @@ default boolean hasArrayOutput(LambdaExpr lambdaExpr)
    */
   void validateArguments(LambdaExpr lambdaExpr, List<Expr> args);
 
+  @Nullable
+  ExprType getOutputType(Expr.InputBindingTypes inputTypes, LambdaExpr expr, List<Expr> args);

Review comment:
       javadocs please

##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -848,4 +882,32 @@ public CartesianFoldLambdaBinding accumulateWithIndex(int index, Object acc)
       return this;
     }
   }
+
+  class LambdaInputBindingTypes implements Expr.InputBindingTypes

Review comment:
       javadocs please

##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -848,4 +882,32 @@ public CartesianFoldLambdaBinding accumulateWithIndex(int index, Object acc)
       return this;
     }
   }
+
+  class LambdaInputBindingTypes implements Expr.InputBindingTypes
+  {
+    private final Object2IntMap<String> lambdaIdentifiers;
+    private final Expr.InputBindingTypes inputTypes;
+    private final List<Expr> args;
+
+    public LambdaInputBindingTypes(Expr.InputBindingTypes inputTypes, LambdaExpr expr, List<Expr> args)
+    {
+      this.inputTypes = inputTypes;
+      this.args = args;
+      List<String> identifiers = expr.getIdentifiers();
+      this.lambdaIdentifiers = new Object2IntOpenHashMap<>(args.size());
+      for (int i = 0; i < args.size(); i++) {
+        lambdaIdentifiers.put(identifiers.get(i), i);
+      }
+    }
+
+    @Nullable
+    @Override
+    public ExprType getType(String name)
+    {
+      if (lambdaIdentifiers.containsKey(name)) {
+        return ExprType.elementType(args.get(lambdaIdentifiers.getInt(name)).getOutputType(inputTypes));

Review comment:
       Willl getType be called in a loop anywhere? It might be better to use `getOrDefault(..)` instead to avoid 2 hashcode computations here (for `containsKey` and `getInt`)

##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -57,6 +57,17 @@ protected final double evalDouble(double left, double right)
     // Use Double.compare for more consistent NaN handling.
     return Evals.asDouble(Double.compare(left, right) < 0);
   }
+
+  @Nullable
+  @Override
+  public ExprType getOutputType(InputBindingTypes inputTypes)
+  {
+    ExprType implicitCast = super.getOutputType(inputTypes);
+    if (ExprType.STRING.equals(implicitCast)) {
+      return ExprType.LONG;
+    }
+    return implicitCast;

Review comment:
       Looking at how this function works got me thinking about some stuff... Does this function need to be in sync with the behavior in `BinaryEvalOpExprBase#eval` (I think so 🤔) Since the `eval` method isn't implemented here, would it be better to implement it in `BinaryEvalOpExprBase`?
   
   Can you explain how `getOutputType` would deal with default null handling mode.
   
   Also, what does it mean to have an output type of null?




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

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



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