abhishekagarwal87 commented on a change in pull request #10350: URL: https://github.com/apache/druid/pull/10350#discussion_r486929873
########## File path: processing/src/main/java/org/apache/druid/query/expression/ContainsExpr.java ########## @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.query.expression; + +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; +import org.apache.druid.math.expr.ExprEval; +import org.apache.druid.math.expr.ExprMacroTable; +import org.apache.druid.math.expr.ExprType; + +import javax.annotation.Nonnull; +import java.util.function.Function; + +/** + * {@link Expr} class returned by {@link ContainsExprMacro} and {@link CaseInsensitiveContainsExprMacro} for + * evaluating the expression. + */ +class ContainsExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr +{ + private final Function<String, Boolean> searchFunction; + private final Expr searchStrExpr; Review comment: `searchStrExpr` is required in `stringify` method. ########## File path: sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java ########## @@ -1072,6 +1075,221 @@ public void testPad() ); } + @Test + public void testContains() + { + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'There')"), + 0L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(\"spacey\",'There')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("what") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'what')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(concat('what is',\"spacey\"),'There')"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(contains_string(\"spacey\",'there') && ('yes' == 'yes'))"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(icontains_string(\"spacey\",'There') && ('yes' == 'yes'))"), Review comment: Added two tests for the same. Learnt a few things along the way. I was not at all expecting an empty string to be turned to `null`. Do you know why that is done? ########## File path: sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java ########## @@ -1072,6 +1075,221 @@ public void testPad() ); } + @Test + public void testContains() + { + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'There')"), + 0L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(\"spacey\",'There')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("what") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'what')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(concat('what is',\"spacey\"),'There')"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(contains_string(\"spacey\",'there') && ('yes' == 'yes'))"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(icontains_string(\"spacey\",'There') && ('yes' == 'yes'))"), Review comment: hmm. what do you think should be the output of following in the default mode assuming `myColumn` has empty value? ``` CONTAINS("myColumn", '') ``` ########## File path: sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java ########## @@ -1072,6 +1075,221 @@ public void testPad() ); } + @Test + public void testContains() + { + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'There')"), + 0L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(\"spacey\",'There')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("what") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'what')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(concat('what is',\"spacey\"),'There')"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(contains_string(\"spacey\",'there') && ('yes' == 'yes'))"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(icontains_string(\"spacey\",'There') && ('yes' == 'yes'))"), Review comment: I was not handling the null values similar to other expressions. It should work now. code coverage bot was complaining as tests were added in `sql` module and these `Expr*` classes are in the `processing` module. After I added the tests in the processing module, I realized what the problem was. When an empty string is passed, the functions get a null value that they again convert to an empty value. ########## File path: processing/src/test/java/org/apache/druid/query/expression/ContainsExprMacroTest.java ########## @@ -0,0 +1,142 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.query.expression; + +import com.google.common.collect.ImmutableMap; +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.math.expr.ExprEval; +import org.apache.druid.math.expr.ExprType; +import org.apache.druid.math.expr.Parser; +import org.junit.Assert; +import org.junit.Test; + +public class ContainsExprMacroTest extends MacroTestBase +{ + public ContainsExprMacroTest() + { + super(new ContainsExprMacro()); + } + + @Test + public void testErrorZeroArguments() + { + expectException(IllegalArgumentException.class, "Function[contains_string] must have 2 arguments"); + eval("contains_string()", Parser.withMap(ImmutableMap.of())); + } + + @Test + public void testErrorThreeArguments() + { + expectException(IllegalArgumentException.class, "Function[contains_string] must have 2 arguments"); + eval("contains_string('a', 'b', 'c')", Parser.withMap(ImmutableMap.of())); + } + + @Test + public void testMatch() + { + final ExprEval<?> result = eval("contains_string(a, 'oba')", Parser.withMap(ImmutableMap.of("a", "foobar"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNoMatch() + { + final ExprEval<?> result = eval("contains_string(a, 'bar')", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(false, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearch() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testEmptyStringSearch() + { + final ExprEval<?> result = eval("contains_string(a, '')", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearchOnEmptyString() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withMap(ImmutableMap.of("a", ""))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testEmptyStringSearchOnEmptyString() + { + final ExprEval<?> result = eval("contains_string(a, '')", Parser.withMap(ImmutableMap.of("a", ""))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearchOnNull() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withSuppliers(ImmutableMap.of("a", () -> null))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), Review comment: In non-sql compatible mode, both of these will be empty instead of null and hence the function would return `true`. In SQL compatible mode, we would see an exception since `null` is not a valid literal. ########## File path: processing/src/test/java/org/apache/druid/query/expression/ContainsExprMacroTest.java ########## @@ -0,0 +1,142 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.query.expression; + +import com.google.common.collect.ImmutableMap; +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.math.expr.ExprEval; +import org.apache.druid.math.expr.ExprType; +import org.apache.druid.math.expr.Parser; +import org.junit.Assert; +import org.junit.Test; + +public class ContainsExprMacroTest extends MacroTestBase +{ + public ContainsExprMacroTest() + { + super(new ContainsExprMacro()); + } + + @Test + public void testErrorZeroArguments() + { + expectException(IllegalArgumentException.class, "Function[contains_string] must have 2 arguments"); + eval("contains_string()", Parser.withMap(ImmutableMap.of())); + } + + @Test + public void testErrorThreeArguments() + { + expectException(IllegalArgumentException.class, "Function[contains_string] must have 2 arguments"); + eval("contains_string('a', 'b', 'c')", Parser.withMap(ImmutableMap.of())); + } + + @Test + public void testMatch() + { + final ExprEval<?> result = eval("contains_string(a, 'oba')", Parser.withMap(ImmutableMap.of("a", "foobar"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNoMatch() + { + final ExprEval<?> result = eval("contains_string(a, 'bar')", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(false, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearch() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testEmptyStringSearch() + { + final ExprEval<?> result = eval("contains_string(a, '')", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearchOnEmptyString() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withMap(ImmutableMap.of("a", ""))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testEmptyStringSearchOnEmptyString() + { + final ExprEval<?> result = eval("contains_string(a, '')", Parser.withMap(ImmutableMap.of("a", ""))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearchOnNull() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withSuppliers(ImmutableMap.of("a", () -> null))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), Review comment: though I am still evaluating if the SQL operator behaves differently in non-sql compatible mode. That is, If `null` is not translated into empty string in the SQL operator, we may see a behaviour mismatch. ########## File path: processing/src/main/java/org/apache/druid/query/expression/ContainsExpr.java ########## @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.query.expression; + +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; +import org.apache.druid.math.expr.ExprEval; +import org.apache.druid.math.expr.ExprMacroTable; +import org.apache.druid.math.expr.ExprType; + +import javax.annotation.Nonnull; +import java.util.function.Function; + +/** + * {@link Expr} class returned by {@link ContainsExprMacro} and {@link CaseInsensitiveContainsExprMacro} for + * evaluating the expression. + */ +class ContainsExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr +{ + private final Function<String, Boolean> searchFunction; + private final Expr searchStrExpr; Review comment: `searchStrExpr` is required in `stringify` method. ########## File path: sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java ########## @@ -1072,6 +1075,221 @@ public void testPad() ); } + @Test + public void testContains() + { + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'There')"), + 0L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(\"spacey\",'There')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("what") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'what')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(concat('what is',\"spacey\"),'There')"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(contains_string(\"spacey\",'there') && ('yes' == 'yes'))"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(icontains_string(\"spacey\",'There') && ('yes' == 'yes'))"), Review comment: Added two tests for the same. Learnt a few things along the way. I was not at all expecting an empty string to be turned to `null`. Do you know why that is done? ########## File path: sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java ########## @@ -1072,6 +1075,221 @@ public void testPad() ); } + @Test + public void testContains() + { + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'There')"), + 0L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(\"spacey\",'There')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("what") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'what')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(concat('what is',\"spacey\"),'There')"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(contains_string(\"spacey\",'there') && ('yes' == 'yes'))"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(icontains_string(\"spacey\",'There') && ('yes' == 'yes'))"), Review comment: hmm. what do you think should be the output of following in the default mode assuming `myColumn` has empty value? ``` CONTAINS("myColumn", '') ``` ########## File path: sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java ########## @@ -1072,6 +1075,221 @@ public void testPad() ); } + @Test + public void testContains() + { + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("contains_string(\"spacey\",'There')"), + 0L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(\"spacey\",'There')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("what") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'what')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("there") + ), + DruidExpression.fromExpression("contains_string(concat('what is',\"spacey\"),'there')"), + 1L + ); + + testHelper.testExpression( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeCall( + SqlStdOperatorTable.CONCAT, + testHelper.makeLiteral("what is"), + testHelper.makeInputRef("spacey") + ), + testHelper.makeLiteral("There") + ), + DruidExpression.fromExpression("icontains_string(concat('what is',\"spacey\"),'There')"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("there") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(contains_string(\"spacey\",'there') && ('yes' == 'yes'))"), + 1L + ); + + testHelper.testExpression( + SqlStdOperatorTable.AND, + ImmutableList.of( + testHelper.makeCall( + ContainsOperatorConversion.caseInsensitive().calciteOperator(), + testHelper.makeInputRef("spacey"), + testHelper.makeLiteral("There") + ), + testHelper.makeCall( + SqlStdOperatorTable.EQUALS, + testHelper.makeLiteral("yes"), + testHelper.makeLiteral("yes") + ) + ), + DruidExpression.fromExpression("(icontains_string(\"spacey\",'There') && ('yes' == 'yes'))"), Review comment: I was not handling the null values similar to other expressions. It should work now. code coverage bot was complaining as tests were added in `sql` module and these `Expr*` classes are in the `processing` module. After I added the tests in the processing module, I realized what the problem was. When an empty string is passed, the functions get a null value that they again convert to an empty value. ########## File path: processing/src/test/java/org/apache/druid/query/expression/ContainsExprMacroTest.java ########## @@ -0,0 +1,142 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.query.expression; + +import com.google.common.collect.ImmutableMap; +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.math.expr.ExprEval; +import org.apache.druid.math.expr.ExprType; +import org.apache.druid.math.expr.Parser; +import org.junit.Assert; +import org.junit.Test; + +public class ContainsExprMacroTest extends MacroTestBase +{ + public ContainsExprMacroTest() + { + super(new ContainsExprMacro()); + } + + @Test + public void testErrorZeroArguments() + { + expectException(IllegalArgumentException.class, "Function[contains_string] must have 2 arguments"); + eval("contains_string()", Parser.withMap(ImmutableMap.of())); + } + + @Test + public void testErrorThreeArguments() + { + expectException(IllegalArgumentException.class, "Function[contains_string] must have 2 arguments"); + eval("contains_string('a', 'b', 'c')", Parser.withMap(ImmutableMap.of())); + } + + @Test + public void testMatch() + { + final ExprEval<?> result = eval("contains_string(a, 'oba')", Parser.withMap(ImmutableMap.of("a", "foobar"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNoMatch() + { + final ExprEval<?> result = eval("contains_string(a, 'bar')", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(false, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearch() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testEmptyStringSearch() + { + final ExprEval<?> result = eval("contains_string(a, '')", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearchOnEmptyString() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withMap(ImmutableMap.of("a", ""))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testEmptyStringSearchOnEmptyString() + { + final ExprEval<?> result = eval("contains_string(a, '')", Parser.withMap(ImmutableMap.of("a", ""))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearchOnNull() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withSuppliers(ImmutableMap.of("a", () -> null))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), Review comment: In non-sql compatible mode, both of these will be empty instead of null and hence the function would return `true`. In SQL compatible mode, we would see an exception since `null` is not a valid literal. ########## File path: processing/src/test/java/org/apache/druid/query/expression/ContainsExprMacroTest.java ########## @@ -0,0 +1,142 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.query.expression; + +import com.google.common.collect.ImmutableMap; +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.math.expr.ExprEval; +import org.apache.druid.math.expr.ExprType; +import org.apache.druid.math.expr.Parser; +import org.junit.Assert; +import org.junit.Test; + +public class ContainsExprMacroTest extends MacroTestBase +{ + public ContainsExprMacroTest() + { + super(new ContainsExprMacro()); + } + + @Test + public void testErrorZeroArguments() + { + expectException(IllegalArgumentException.class, "Function[contains_string] must have 2 arguments"); + eval("contains_string()", Parser.withMap(ImmutableMap.of())); + } + + @Test + public void testErrorThreeArguments() + { + expectException(IllegalArgumentException.class, "Function[contains_string] must have 2 arguments"); + eval("contains_string('a', 'b', 'c')", Parser.withMap(ImmutableMap.of())); + } + + @Test + public void testMatch() + { + final ExprEval<?> result = eval("contains_string(a, 'oba')", Parser.withMap(ImmutableMap.of("a", "foobar"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNoMatch() + { + final ExprEval<?> result = eval("contains_string(a, 'bar')", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(false, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearch() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testEmptyStringSearch() + { + final ExprEval<?> result = eval("contains_string(a, '')", Parser.withMap(ImmutableMap.of("a", "foo"))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearchOnEmptyString() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withMap(ImmutableMap.of("a", ""))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testEmptyStringSearchOnEmptyString() + { + final ExprEval<?> result = eval("contains_string(a, '')", Parser.withMap(ImmutableMap.of("a", ""))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), + result.value() + ); + } + + @Test + public void testNullSearchOnNull() + { + if (NullHandling.sqlCompatible()) { + expectException(IllegalArgumentException.class, "Function[contains_string] substring must be a string literal"); + } + + final ExprEval<?> result = eval("contains_string(a, null)", Parser.withSuppliers(ImmutableMap.of("a", () -> null))); + Assert.assertEquals( + ExprEval.of(true, ExprType.LONG).value(), Review comment: though I am still evaluating if the SQL operator behaves differently in non-sql compatible mode. That is, If `null` is not translated into empty string in the SQL operator, we may see a behaviour mismatch. ---------------------------------------------------------------- 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: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
