[GitHub] [druid] cheddar commented on a diff in pull request #13209: use object[] instead of string[] for vector expressions to be consistent with vector object selectors

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

cheddar commented on code in PR #13209:
URL: https://github.com/apache/druid/pull/13209#discussion_r992870933


##########
processing/src/main/java/org/apache/druid/segment/virtual/SingleStringInputDeferredEvaluationExpressionDimensionVectorSelector.java:
##########
@@ -118,7 +119,7 @@ public int getCurrentVectorSize()
    */
   private static final class StringLookupVectorInputBindings implements Expr.VectorInputBinding
   {
-    private final String[] currentValue = new String[1];
+    private final Object[] currentValue = new Object[1];

Review Comment:
   Nitish: The naming of this private static class doesn't really line up with its implementation anymore?



##########
core/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java:
##########
@@ -851,17 +861,19 @@ public void processIndex(
                 outputNulls[i] = true;
                 return;
               }
-              final boolean bool = Evals.asBoolean(rightInput[i]);
+              final boolean bool = Evals.asBoolean((String) rightInput[i]);
               output[i] = Evals.asLong(bool);
               outputNulls[i] = bool;
               return;
             } else if (rightNull) {
-              final boolean bool = Evals.asBoolean(leftInput[i]);
+              final boolean bool = Evals.asBoolean((String) leftInput[i]);

Review Comment:
   Here too, why is it safe to cast?  do we need to check the type first?  Or maybe check the type of the array that we are gotten, so we can amortize the cost of the check?



##########
core/src/main/java/org/apache/druid/math/expr/vector/VectorStringProcessors.java:
##########
@@ -32,32 +33,35 @@ public static <T> ExprVectorProcessor<T> concat(Expr.VectorInputBindingInspector
   {
     final ExprVectorProcessor processor;
     if (NullHandling.sqlCompatible()) {
-      processor = new StringOutStringsInFunctionVectorProcessor(
+      processor = new ObjectOutObjectsInFunctionVectorProcessor(
           left.buildVectorized(inspector),
           right.buildVectorized(inspector),
-          inspector.getMaxVectorSize()
+          inspector.getMaxVectorSize(),
+          ExpressionType.STRING
       )
       {
         @Nullable
         @Override
-        protected String processValue(@Nullable String leftVal, @Nullable String rightVal)
+        protected String processValue(@Nullable Object leftVal, @Nullable Object rightVal)
         {
           // in sql compatible mode, nulls are handled by super class and never make it here...
-          return leftVal + rightVal;
+          return leftVal + (String) rightVal;

Review Comment:
   You could call `.toString()` and avoid ClassCastExceptions here?  Or, if it's important to do the cast, we should really do an instanceof check first so that we can generate a better error message than just `ClassCastException`



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

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