clintropolis commented on code in PR #18084:
URL: https://github.com/apache/druid/pull/18084#discussion_r2131033928
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java:
##########
@@ -316,6 +316,7 @@ public void testMultiValueStringOverlapFilterNonLiteral()
),
ImmutableList.of(
new Object[]{"[\"a\",\"b\"]"},
+ new Object[]{"[\"b\",\"c\"]"},
Review Comment:
i suppose this test should have also selected dim2 to make it easier to
verify heh, though I'm still kind of confused, what does the array constructor
end up doing to an empty mvd row? I started to try to find out but then ran
into a wierd problem with the grammar, you can't actually write
`array(array(...))` expression because it gets confused with some other junk in
there about array literal parsing.. will look into both of these things later
and just assume this is correct.
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/MultiValueStringOperatorConversions.java:
##########
@@ -169,9 +168,9 @@ public SqlOperator calciteOperator()
}
@Override
- protected String getFilterExpression(List<DruidExpression>
druidExpressions)
+ public String getDruidFunctionName()
Review Comment:
should this still extend ArrayContainsOperatorConversion? I guess i see the
benefit of overlapping filter conversion, but given that it uses some other
native filters, such as `ArrayContainsElementFilter`, is that ok?
##########
processing/src/test/java/org/apache/druid/math/expr/FunctionTest.java:
##########
@@ -391,6 +391,73 @@ public void testArrayContains()
assertExpr("array_contains([1, null, 2], null)", 1L);
assertExpr("array_contains([1, null, 2], [null])", 1L);
assertExpr("array_contains([1, 2], null)", 0L);
+ assertExpr("array_contains([1, 2, 3], [])", 1L);
+ assertExpr("array_contains([], [])", 1L);
+ assertExpr("array_contains([null], [])", 1L);
+ assertExpr("array_contains(['foo', 'bar'], [])", 1L);
+ assertExpr("array_contains(null, [])", null);
+ }
+
+ @Test
+ public void testMvContains()
Review Comment:
can you add some tests for what happens if other types of columns make it in
here, like scalar numbers and number arrays and stuff? i think should be fine,
just might be nice to have.. see there are tests for scalar strings which is
:+1: just would like to see a bit more (and also maybe as bindings since
different paths for singleThreaded for constant and dynamic args)
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/MultiValueStringOperatorConversions.java:
##########
@@ -403,14 +402,12 @@ public DruidExpression toDruidExpression(
plannerContext,
rowSignature,
rexNode,
- druidExpressions -> {
- final List<DruidExpression> newArgs =
harmonizeNullsMvdArg0OperandList(druidExpressions);
- return DruidExpression.ofFunctionCall(
- Calcites.getColumnTypeForRelDataType(rexNode.getType()),
- getDruidFunctionName(),
- newArgs
- );
- }
+ druidExpressions ->
Review Comment:
this looks like the default implementation, so can just delete i think,
unless i'm missing something?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
