You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/06/10 15:23:30 UTC
[GitHub] [calcite] yinchuanwang opened a new pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
yinchuanwang opened a new pull request #2434:
URL: https://github.com/apache/calcite/pull/2434
This PR is the following action of [PR2420](https://github.com/apache/calcite/pull/2420) to implement the translation from range search to range query in elastic search adapter. One specific test class "SearchTest" is added to test the translation from "Search" to terms/range query.
--
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:
users@infra.apache.org
[GitHub] [calcite] ILuffZhe commented on pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
ILuffZhe commented on pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#issuecomment-1004918303
Hi, @yinchuanwang ! Are you still working on this patch?
I've disabled testFilterSortDesc() in ElasticsearchAdapterTest.java through https://github.com/apache/calcite/pull/2659, which could be solved in this patch. Please take a look if you have time, that will be very helpful!
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yinchuanwang commented on a change in pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
yinchuanwang commented on a change in pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#discussion_r650946777
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java
##########
@@ -873,8 +866,37 @@ private SimpleQueryExpression(NamedFieldExpression rel) {
builder = boolQuery().mustNot(termsQuery(getFieldReference(), iterable));
return this;
}
- }
+ @Override public QueryExpression range(LiteralExpression literal) {
+ Iterator<?> iterator = ((Iterable<?>) literal.value()).iterator();
+ BoolQueryBuilder boolQuery = boolQuery();
+ while (iterator.hasNext()) {
+ Range range = (Range) iterator.next();
Review comment:
Originally I would like to add a class 'RangeExpression' to convey the information of 'Range Set'. When i began to implement it, i found the 'RangeExpression''s structure will be almost same with the class 'Range'. The benefit to add a new RangeExpression looks not that much. So i just directly return a list of 'Range' and build the rangeQueryBuilder for each Range. However you are right and I should at least move the extraction and transformation of the Range to a kind of utility function.
--
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:
users@infra.apache.org
[GitHub] [calcite] yinchuanwang commented on pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
yinchuanwang commented on pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#issuecomment-879603666
Hello @amaliujia, It seems that @zinking is not available recently. Do you have time to help review this 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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#issuecomment-880074490
Sure I can try to start taking a look within this week.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#discussion_r671490909
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/SearchTest.java
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.calcite.adapter.elasticsearch;
Review comment:
Why choose to create a new class for Search?
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zinking commented on a change in pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
zinking commented on a change in pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#discussion_r649712956
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java
##########
@@ -200,22 +201,6 @@ private static boolean supportedRexCall(RexCall call) {
}
}
- /**
Review comment:
I'd suggest keep the comments but modify them slightly
so that I can still grab the context regarding `sarg`
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java
##########
@@ -741,6 +730,10 @@ private CompoundQueryExpression(boolean partial, BoolQueryBuilder builder) {
@Override public QueryExpression notIn(LiteralExpression literal) {
throw new PredicateAnalyzerException("notIn cannot be applied to a compound expression");
}
+
+ @Override public QueryExpression range(LiteralExpression literal) {
Review comment:
I thought all `sargs` are supported now, aren't they?
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java
##########
@@ -873,8 +866,37 @@ private SimpleQueryExpression(NamedFieldExpression rel) {
builder = boolQuery().mustNot(termsQuery(getFieldReference(), iterable));
return this;
}
- }
+ @Override public QueryExpression range(LiteralExpression literal) {
+ Iterator<?> iterator = ((Iterable<?>) literal.value()).iterator();
+ BoolQueryBuilder boolQuery = boolQuery();
+ while (iterator.hasNext()) {
+ Range range = (Range) iterator.next();
Review comment:
I don't have much context, but usually I would assume it should be some RangeExpression, or at least some utils to extract the Range expressions, instead of expose the iterator directly here?
would that be better ?
--
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:
users@infra.apache.org
[GitHub] [calcite] yinchuanwang commented on a change in pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
yinchuanwang commented on a change in pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#discussion_r650915698
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java
##########
@@ -741,6 +730,10 @@ private CompoundQueryExpression(boolean partial, BoolQueryBuilder builder) {
@Override public QueryExpression notIn(LiteralExpression literal) {
throw new PredicateAnalyzerException("notIn cannot be applied to a compound expression");
}
+
+ @Override public QueryExpression range(LiteralExpression literal) {
Review comment:
Yes.
--
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:
users@infra.apache.org
[GitHub] [calcite] yinchuanwang commented on a change in pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
yinchuanwang commented on a change in pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#discussion_r671580495
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/SearchTest.java
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.calcite.adapter.elasticsearch;
Review comment:
The main purpose of 'AggregationTest' is for aggregation tests which focus on the aggregation logic. With this PR, 2 search test cases (8 tests) are added. Maybe it is a little bit confusing to still leave them in 'AggregationTest'
Another reason is the test data of 'Search' is a little bit different from 'Aggregation' for covering more scenarios of 'Search to Range'. In order to avoid to break the existing test cases of 'Aggregation', so I created a separate test class for Search.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zinking commented on pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
zinking commented on pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#issuecomment-859301106
looks in pretty good shape.
--
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:
users@infra.apache.org
[GitHub] [calcite] yinchuanwang commented on pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
yinchuanwang commented on pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#issuecomment-864808254
> looks in pretty good shape.
I have updated the code based on your advice. Any other comments?
--
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:
users@infra.apache.org
[GitHub] [calcite] yinchuanwang commented on pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
yinchuanwang commented on pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#issuecomment-864808254
> looks in pretty good shape.
I have updated the code based on your advice. Any other comments?
--
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:
users@infra.apache.org
[GitHub] [calcite] yinchuanwang commented on pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
yinchuanwang commented on pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#issuecomment-880290067
Many thanks.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yinchuanwang commented on a change in pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
yinchuanwang commented on a change in pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#discussion_r650915318
##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/PredicateAnalyzer.java
##########
@@ -200,22 +201,6 @@ private static boolean supportedRexCall(RexCall call) {
}
}
- /**
Review comment:
Ok. I kept the the explanation about the 3 subtypes of Sarg in the comments.
--
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:
users@infra.apache.org
[GitHub] [calcite] yinchuanwang commented on pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
yinchuanwang commented on pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#issuecomment-866540651
Hello @zinking, do you have time to review the change?
--
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:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #2434: [CALCITE-4645] In Elasticsearch adapter, translate a range predicate to a range query(Jacky Yin)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2434:
URL: https://github.com/apache/calcite/pull/2434#discussion_r674203237
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/SearchTest.java
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.calcite.adapter.elasticsearch;
Review comment:
Thanks. PR overall LGTM. I will take another pass soon.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org