You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "gianm (via GitHub)" <gi...@apache.org> on 2025/06/05 21:24:58 UTC
[PR] Add dedicated mv_overlap, mv_contains functions. (druid)
gianm opened a new pull request, #18084:
URL: https://github.com/apache/druid/pull/18084
Introduces new multi-value string functions at the native layer. These are now used for the SQL `MV_OVERLAP` and `MV_CONTAINS` functions. The main purpose of the new functions is to have slightly different null and empty-array handling behavior vs `array_overlap` and `array_contains`. Behavior is designed to align with the native `inType` filter, which is especially important for `MV_OVERLAP`, because the SQL layer can convert `MV_OVERLAP` into an `inType` filter.
The SQL layer no longer generates calls to the `mv_harmonize_nulls` function, because that logic has been moved inside `mv_overlap` and `mv_contains`. The function still remains at the native layer for backwards compatibility.
--
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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18084:
URL: https://github.com/apache/druid/pull/18084#discussion_r2131547686
##########
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:
The main reason for extending `ArrayContainsOperatorConversion` is to get the filter conversion behavior. This patch doesn't change whether `ArrayContainsElementFilter` could fire for `MV_CONTAINS`, but just to be safe, I added a check for this. (If the operator is not `ARRAY_CONTAINS` then it skips the array-element-filter block.)
--
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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18084:
URL: https://github.com/apache/druid/pull/18084#discussion_r2132006112
##########
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:
This actually was a bad change. The root cause was:
- the `array(dim2)` function, when passed a `dim2` that is a single null, would return an `array<long>` typed array with a single null in it.
- the `mv_overlap` function would then cast the first arg to `array<long>`, which would yield an array of nulls.
- the `mv_overlap` function would then return true because an array of nulls (first arg) does contain a null (second arg).
To fix it we'd need to cast for comparison. But I think that is sort of pointless since `MV_CONTAINS` and `MV_OVERLAP` really only make sense with strings. So instead, I updated them to cast the second arg to string array, and updated the tests accordingly.
--
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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18084:
URL: https://github.com/apache/druid/pull/18084#discussion_r2132091841
##########
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:
Ah, I had deleted the other one, not this one you commented on. Just deleted this one too. Actually, I did a bigger change:
- updated MV Contains and Overlap to call a superclass constructor with their operator and function names, rather than overriding `calciteOperator()` and `getDruidFunctionName()`
- removed `toDruidExpression` and `toDruidExpressionWithPostAggOperands` from MV Contains and Overlap
- marked `calciteOperator()` and `getDruidFunctionName()` as `final` in `DirectOperatorConversion`, so all the *other* methods on `DirectOperatorConversion` can be trusted to use the correct operator and function name
--
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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #18084:
URL: https://github.com/apache/druid/pull/18084
--
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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18084:
URL: https://github.com/apache/druid/pull/18084#discussion_r2131538028
##########
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:
I think you're right, will delete it.
--
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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #18084:
URL: https://github.com/apache/druid/pull/18084#discussion_r2130594671
##########
processing/src/main/java/org/apache/druid/math/expr/Function.java:
##########
@@ -4157,6 +4158,300 @@
}
}
+ class MvOverlapFunction implements Function
+ {
+ @Override
+ public String name()
+ {
+ return "mv_overlap";
+ }
+
+ @Nullable
+ @Override
+ public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args)
+ {
+ return ExpressionType.LONG;
+ }
+
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final ExprEval<?> arg1 = Function.harmonizeMultiValue(args.get(0).eval(bindings));
+ final ExprEval<?> arg2 = args.get(1).eval(bindings);
+
+ // Cast arg1 to arg2's type.
+ final Object[] array2 = arg2.asArray();
+ final ExpressionType array2Type = arg2.asArrayType();
+ final Object[] array1 = arg1.castTo(array2Type).asArray();
+
+ // If the second argument is null, check if the first argument contains null.
+ if (array2 == null) {
+ return ExprEval.ofLongBoolean(arrayContainsNull(array1));
+ }
+
+ // If the second argument is empty array, return false regardless of first argument.
+ if (array2.length == 0) {
+ return ExprEval.ofLongBoolean(false);
+ }
+
+ // Check for overlap.
+ final Set<Object> set2 = new ObjectOpenHashSet<>(array2);
+ for (final Object check : array1) {
+ if (set2.contains(check)) {
+ return ExprEval.ofLongBoolean(true);
+ }
+ }
+
+ // No overlap.
+ if (!set2.contains(null) && arrayContainsNull(array1)) {
+ return ExprEval.ofLong(null);
+ } else {
+ return ExprEval.ofLongBoolean(false);
+ }
+ }
+
+ @Override
+ public void validateArguments(List<Expr> args)
+ {
+ validationHelperCheckArgumentCount(args, 2);
+ }
+
+ @Override
+ public Set<Expr> getScalarInputs(List<Expr> args)
+ {
+ return Collections.emptySet();
+ }
+
+ @Override
+ public Set<Expr> getArrayInputs(List<Expr> args)
+ {
+ return ImmutableSet.copyOf(args);
+ }
+
+ @Override
Review Comment:
## Unread local variable
Variable 'Expr arg1' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/9209)
##########
processing/src/main/java/org/apache/druid/math/expr/Function.java:
##########
@@ -4157,6 +4158,300 @@
}
}
+ class MvOverlapFunction implements Function
+ {
+ @Override
+ public String name()
+ {
+ return "mv_overlap";
+ }
+
+ @Nullable
+ @Override
+ public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args)
+ {
+ return ExpressionType.LONG;
+ }
+
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final ExprEval<?> arg1 = Function.harmonizeMultiValue(args.get(0).eval(bindings));
+ final ExprEval<?> arg2 = args.get(1).eval(bindings);
+
+ // Cast arg1 to arg2's type.
+ final Object[] array2 = arg2.asArray();
+ final ExpressionType array2Type = arg2.asArrayType();
+ final Object[] array1 = arg1.castTo(array2Type).asArray();
+
+ // If the second argument is null, check if the first argument contains null.
+ if (array2 == null) {
+ return ExprEval.ofLongBoolean(arrayContainsNull(array1));
+ }
+
+ // If the second argument is empty array, return false regardless of first argument.
+ if (array2.length == 0) {
+ return ExprEval.ofLongBoolean(false);
+ }
+
+ // Check for overlap.
+ final Set<Object> set2 = new ObjectOpenHashSet<>(array2);
+ for (final Object check : array1) {
+ if (set2.contains(check)) {
+ return ExprEval.ofLongBoolean(true);
+ }
+ }
+
+ // No overlap.
+ if (!set2.contains(null) && arrayContainsNull(array1)) {
+ return ExprEval.ofLong(null);
+ } else {
+ return ExprEval.ofLongBoolean(false);
+ }
+ }
+
+ @Override
+ public void validateArguments(List<Expr> args)
+ {
+ validationHelperCheckArgumentCount(args, 2);
+ }
+
+ @Override
+ public Set<Expr> getScalarInputs(List<Expr> args)
+ {
+ return Collections.emptySet();
+ }
+
+ @Override
+ public Set<Expr> getArrayInputs(List<Expr> args)
+ {
+ return ImmutableSet.copyOf(args);
+ }
+
+ @Override
+ public boolean hasArrayInputs()
+ {
+ return true;
+ }
+
+ @Override
+ public Function asSingleThreaded(List<Expr> args, Expr.InputBindingInspector inspector)
+ {
+ final Expr arg1 = args.get(0);
+ final Expr arg2 = args.get(1);
+
+ if (arg2.isLiteral()) {
+ final ExprEval<?> rhsEval = args.get(1).eval(InputBindings.nilBindings());
+ final Object[] rhsArray = rhsEval.asArray();
+
+ if (rhsArray == null) {
+ return new MvOverlapConstantNull();
+ } else if (rhsArray.length == 0) {
+ return new MvOverlapConstantEmpty();
+ } else if (rhsEval.elementType().isPrimitive()) {
+ return new MvOverlapConstantArray(
+ new ObjectOpenHashSet<>(rhsArray),
+ arrayContainsNull(rhsArray),
+ rhsEval.asArrayType()
+ );
+ }
+ }
+
+ return this;
+ }
+
+ private static final class MvOverlapConstantNull extends MvOverlapFunction
+ {
+ @Override
+ public ExprEval<?> apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final ExprEval<?> arrayExpr1 = Function.harmonizeMultiValue(args.get(0).eval(bindings));
+ return ExprEval.ofLongBoolean(arrayContainsNull(arrayExpr1.asArray()));
+ }
+ }
+
+ private static final class MvOverlapConstantEmpty extends MvOverlapFunction
+ {
+ @Override
+ public ExprEval<?> apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ return ExprEval.ofLongBoolean(false);
+ }
+ }
+
+ private static final class MvOverlapConstantArray extends MvOverlapFunction
+ {
+ final Set<Object> matchValues;
+ final boolean rhsHasNull;
+ final ExpressionType matchArrayType;
+
+ public MvOverlapConstantArray(Set<Object> matchValues, boolean rhsHasNull, ExpressionType matchArrayType)
+ {
+ this.matchValues = matchValues;
+ this.rhsHasNull = rhsHasNull;
+ this.matchArrayType = matchArrayType;
+ }
+
+ @Override
+ public ExprEval<?> apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final ExprEval<?> arrayExpr1 = Function.harmonizeMultiValue(args.get(0).eval(bindings));
+ final Object[] array1 = arrayExpr1.castTo(matchArrayType).asArray();
+
+ for (final Object check : array1) {
+ if (matchValues.contains(check)) {
+ return ExprEval.ofLongBoolean(true);
+ }
+ }
+
+ // No overlap.
+ if (!rhsHasNull && arrayContainsNull(array1)) {
+ return ExprEval.ofLong(null);
+ } else {
+ return ExprEval.ofLongBoolean(false);
+ }
+ }
+ }
+ }
+
+ class MvContainsFunction implements Function
+ {
+ @Override
+ public String name()
+ {
+ return "mv_contains";
+ }
+
+ @Nullable
+ @Override
+ public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args)
+ {
+ return ExpressionType.LONG;
+ }
+
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final ExprEval<?> arg1 = Function.harmonizeMultiValue(args.get(0).eval(bindings));
+ final ExprEval<?> arg2 = args.get(1).eval(bindings);
+
+ // Cast arg1 to arg2's type.
+ final Object[] array2 = arg2.asArray();
+ final ExpressionType array2Type = arg2.asArrayType();
+ final Object[] array1 = arg1.castTo(array2Type).asArray();
+
+ // If the second argument is null, check if the first argument contains null.
+ if (array2 == null) {
+ return ExprEval.ofLongBoolean(arrayContainsNull(array1));
+ }
+
+ // If the second argument is an empty array, return true regardless of the first argument.
+ if (array2.length == 0) {
+ return ExprEval.ofLongBoolean(true);
+ }
+
+ return ExprEval.ofLongBoolean(Arrays.asList(array1).containsAll(Arrays.asList(array2)));
+ }
+
+ @Override
+ public void validateArguments(List<Expr> args)
+ {
+ validationHelperCheckArgumentCount(args, 2);
+ }
+
+ @Override
+ public Set<Expr> getScalarInputs(List<Expr> args)
+ {
+ return Collections.emptySet();
+ }
+
+ @Override
+ public Set<Expr> getArrayInputs(List<Expr> args)
+ {
+ return ImmutableSet.copyOf(args);
+ }
+
+ @Override
Review Comment:
## Unread local variable
Variable 'Expr arg1' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/9210)
--
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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18084:
URL: https://github.com/apache/druid/pull/18084#discussion_r2131548437
##########
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:
Added some tests. I didn't use bindings, but I don't think they're needed, because `assertExpr` tests both with and without single-threaded mode. Using constants should let both modes be exercised.
--
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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #18084:
URL: https://github.com/apache/druid/pull/18084#discussion_r2131670924
##########
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:
> I didn't use bindings, but I don't think they're needed, because assertExpr tests both with and without single-threaded mode.
oops yea, i forgot it did that :+1:
--
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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #18084:
URL: https://github.com/apache/druid/pull/18084#issuecomment-2950755330
> lgtm, though I didn't like validate the quidem test changes.. just going to trust that the changes there are correct
I think the quidem tests are OK- looking through them, they appear to mainly be differences in how null/empty cases are handled.
--
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
Re: [PR] Add dedicated mv_overlap, mv_contains functions. (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #18084:
URL: https://github.com/apache/druid/pull/18084#discussion_r2131678226
##########
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:
ah, sorry for confusion, i meant of the entire `toDruidExpression ` method override can be dropped i think, since this is effectively the same as `DirectOperatorConversion` implementation
--
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