[GitHub] [druid] cheddar commented on a diff in pull request #13214: fix json_value sql planning with decimal type, fix vectorized expression math null value handling in default mode

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

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


##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/NestedDataOperatorConversions.java:
##########
@@ -276,7 +276,8 @@ public SqlRexConvertlet createConvertlet(PlannerContext plannerContext)
               call.operand(0),
               call.operand(1)
           );
-        } else if (SqlTypeName.APPROX_TYPES.contains(sqlType.getSqlTypeName())) {
+        } else if (SqlTypeName.APPROX_TYPES.contains(sqlType.getSqlTypeName()) ||
+                   SqlTypeName.DECIMAL.equals(sqlType.getSqlTypeName())) {

Review Comment:
   perhaps invert these?  Equality is faster than `.contains()` so will be better to check the equality first and then do the `.contains()` instead of forcing the `.contains()` to fail first before doing the equality.



##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -2003,6 +2032,35 @@ public void testReturningAndSumPathDecimal()
     );
   }
 
+  @Test
+  public void testReturningAndSumPathDecimalWithMaths()
+  {
+    testQuery(

Review Comment:
   How did these tests fail before this change?  The description makes it sound like the failure is actually a failure to pick the vectorized version rather than a failure in actual results, but I don't see this test ensuring that the vectorized version is used?



##########
core/src/main/java/org/apache/druid/math/expr/vector/UnivariateFunctionVectorValueProcessor.java:
##########
@@ -74,5 +76,7 @@ public final ExprEvalVector<TOutput> evalVector(Expr.VectorInputBinding bindings
 
   abstract void processIndex(TInput input, int i);
 
+  abstract void processNull(int i);

Review Comment:
   Instead of another abstract method call, I wonder how far we could get with a `.getValueForNull()` method that return a value that we can set?  With that setup, we would be able to call that once and then reuse the value, allowing for inlining and stuff.  It should work well for the scope of changes in this PR...



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