You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/04/05 10:19:39 UTC
[GitHub] [pinot] atris opened a new pull request, #8466: Implement CONTAINS Operator
atris opened a new pull request, #8466:
URL: https://github.com/apache/pinot/pull/8466
This commit introduces the CONTAINS operator
(https://docs.microsoft.com/en-us/sql/relational-databases/search/query-with-full-text-search?view=sql-server-ver15)
CONTAINS matches tokens vs LIKE which matches entire documents. CONTAINS operates on the text index and has that as a prerequisite.
--
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
[GitHub] [pinot] atris commented on a diff in pull request #8466: Implement CONTAINS Operator
Posted by GitBox <gi...@apache.org>.
atris commented on code in PR #8466:
URL: https://github.com/apache/pinot/pull/8466#discussion_r843457599
##########
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:
I would still argue that we introduce the Lucene index support in this PR. We should not disallow CONTAINS if the user does not have a native text index, but has a Lucene text index
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8466: Implement CONTAINS Operator
Posted by 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
[GitHub] [pinot] Jackie-Jiang commented on pull request #8466: Implement CONTAINS Operator
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8466:
URL: https://github.com/apache/pinot/pull/8466#issuecomment-1092287657
@atris The problem of allowing `CONTAINS` to use lucene text index is that we have to use the lucene query syntax for `CONTAINS` which might not be desired. Also it doesn't add much value because user can already use `TEXT_MATCH` to query lucene text index.
--
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
[GitHub] [pinot] atris closed pull request #8466: Implement CONTAINS Operator
Posted by GitBox <gi...@apache.org>.
atris closed pull request #8466: Implement CONTAINS Operator
URL: https://github.com/apache/pinot/pull/8466
--
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
[GitHub] [pinot] atris commented on a diff in pull request #8466: Implement CONTAINS Operator
Posted by GitBox <gi...@apache.org>.
atris commented on code in PR #8466:
URL: https://github.com/apache/pinot/pull/8466#discussion_r843455850
##########
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:
We will eventually have to take this path. We cannot base an operator on the type of underlying index. IMO CONTAINS will work on top of Lucene text index by converting to text match operator, and for native index, use a new operator.
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #8466: Implement CONTAINS Operator
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8466:
URL: https://github.com/apache/pinot/pull/8466#issuecomment-1088550880
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8466?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8466](https://codecov.io/gh/apache/pinot/pull/8466?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ab682c7) into [master](https://codecov.io/gh/apache/pinot/commit/5e56b9a155b9f5d7ba3cd7214c15b3f1c697a351?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5e56b9a) will **decrease** coverage by `56.52%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head ab682c7 differs from pull request most recent head dc6c20b. Consider uploading reports for the commit dc6c20b to get more accurate results
```diff
@@ Coverage Diff @@
## master #8466 +/- ##
=============================================
- Coverage 70.70% 14.18% -56.53%
+ Complexity 4285 84 -4201
=============================================
Files 1664 1620 -44
Lines 87342 85461 -1881
Branches 13227 13026 -201
=============================================
- Hits 61757 12119 -49638
- Misses 21296 72425 +51129
+ Partials 4289 917 -3372
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.18% <0.00%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ot/common/request/context/RequestContextUtils.java](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9jb250ZXh0L1JlcXVlc3RDb250ZXh0VXRpbHMuamF2YQ==) | `0.00% <0.00%> (-73.04%)` | :arrow_down: |
| [...n/request/context/predicate/ContainsPredicate.java](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9jb250ZXh0L3ByZWRpY2F0ZS9Db250YWluc1ByZWRpY2F0ZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ot/common/request/context/predicate/Predicate.java](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9jb250ZXh0L3ByZWRpY2F0ZS9QcmVkaWNhdGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/pql/parsers/pql2/ast/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9GaWx0ZXJLaW5kLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `0.00% <0.00%> (-87.56%)` | :arrow_down: |
| [...ava/org/apache/pinot/core/plan/FilterPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0ZpbHRlclBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-86.67%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1327 more](https://codecov.io/gh/apache/pinot/pull/8466/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8466?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8466?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5e56b9a...dc6c20b](https://codecov.io/gh/apache/pinot/pull/8466?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] atris commented on a diff in pull request #8466: Implement CONTAINS Operator
Posted by GitBox <gi...@apache.org>.
atris commented on code in PR #8466:
URL: https://github.com/apache/pinot/pull/8466#discussion_r843453406
##########
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:
This is to allow TextMatchFilterOperator to be able to process a CONTAINS 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:
Ack, removed
--
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
[GitHub] [pinot] atris commented on pull request #8466: Implement CONTAINS Operator
Posted by GitBox <gi...@apache.org>.
atris commented on PR #8466:
URL: https://github.com/apache/pinot/pull/8466#issuecomment-1108839825
Merging as part of main text index 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@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