[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8466: Implement CONTAINS Operator

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

Jackie-Jiang commented on code in PR #8466:
URL: https://github.com/apache/pinot/pull/8466#discussion_r843264058


##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/ContainsPredicate.java:
##########
@@ -0,0 +1,66 @@
+/**
+ * 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.pinot.common.request.context.predicate;
+
+import java.util.Objects;
+import org.apache.pinot.common.request.context.ExpressionContext;
+
+
+/**
+ * Represents a CONTAINS predicate.
+ */
+public class ContainsPredicate extends TextMatchPredicate {

Review Comment:
   Why does it extend the `TextMatchPredicate`? It should be an independent predicate



##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -703,6 +703,10 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
         negated = ((SqlLikeOperator) functionNode.getOperator()).isNegated();
         canonicalName = SqlKind.LIKE.name();
         break;
+      case CONTAINS:
+        negated = ((SqlLikeOperator) functionNode.getOperator()).isNegated();

Review Comment:
   (MAJOR) this is not `SqlLikeOperator`, and will cause cast exception. No need to add `CONTAINS` branch into this switch, the default handling should be good for it



##########
pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java:
##########
@@ -195,6 +195,14 @@ private BaseFilterOperator constructPhysicalOperator(FilterContext filter, int n
             return FilterOperatorUtils.getLeafFilterOperator(predicateEvaluator, dataSource, numDocs);
           }
           switch (predicate.getType()) {
+            case CONTAINS:
+              if (dataSource.getTextIndex() != null) {
+                // For now, CONTAINS is executed by the TEXT_MATCH operator. With integration of native text indices
+                // the type of operator used will depend on the type of the underlying text index.
+                return new TextMatchFilterOperator(dataSource.getTextIndex(), (TextMatchPredicate) predicate, numDocs);
+              }
+
+              throw new UnsupportedOperationException("CONTAINS is supported only on text index enabled fields");

Review Comment:
   I'd suggest directly throwing `UnsupportedOperationException` for now before the native text index integration. There is no value and can cause confusion treating it as `TEXT_MATCH`



##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -239,6 +241,9 @@ public static FilterContext getFilter(Expression thriftExpression) {
       case TEXT_MATCH:
         return new FilterContext(FilterContext.Type.PREDICATE, null,
             new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+      case CONTAINS:

Review Comment:
   (minor) Move this following the `LIKE` case because they are similar



##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -337,6 +343,9 @@ public static FilterContext getFilter(ExpressionContext filterExpression) {
       case TEXT_MATCH:
         return new FilterContext(FilterContext.Type.PREDICATE, null,
             new TextMatchPredicate(operands.get(0), getStringValue(operands.get(1))));
+      case CONTAINS:

Review Comment:
   (minor) Move this following the `LIKE` case



##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/LuceneRealtimeClusterIntegrationTest.java:
##########
@@ -58,6 +58,12 @@
 
   private static final String TEST_TEXT_COLUMN_QUERY =
       "SELECT COUNT(*) FROM mytable WHERE TEXT_MATCH(skills, '\"machine learning\" AND spark')";
+  private static final String TEST_CONTAINS_QUERY =

Review Comment:
   Don't add it into this integration test. We can add some tests to the `CalciteSqlCompilerTest` for the syntax support



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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org