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/12/24 15:32:11 UTC
[GitHub] [calcite] ILuffZhe opened a new pull request #2659: [CALCITE-4960] Enable unit tests in Elasticsearch Adapter
ILuffZhe opened a new pull request #2659:
URL: https://github.com/apache/calcite/pull/2659
This PR aims at **re-enabling unit tests** in Elasticsearch Adapter. The changes are:
- Increasing SocketTimeout and ConnectionTimeout for RestClient to avoid CI failure
- Remove one case in BooleanLogicTest.java, which is documented at https://issues.apache.org/jira/browse/CALCITE-4965
- Disable testFilterSortDesc() in ElasticSearchAdapterTest.java, which is not supported before CALCITE-4645 merged.
I think re-enabling those tests has higher priorities than fixing bugs in ES Adapter. When other contributors make changes in this module, their codes cannot be guaranteed well without unit tests. Last but not least, the disabled cases mentioned above will be reopened after bug fixing.
--
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] ILuffZhe commented on a change in pull request #2659: [CALCITE-4960] Enable unit tests in Elasticsearch Adapter
Posted by GitBox <gi...@apache.org>.
ILuffZhe commented on a change in pull request #2659:
URL: https://github.com/apache/calcite/pull/2659#discussion_r775104448
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/ElasticSearchAdapterTest.java
##########
@@ -476,8 +484,8 @@ public static void setupInstance() throws Exception {
@Test void testInPlan() {
final String[] searches = {
- "query: {'constant_score':{filter:{bool:{should:"
Review comment:
After CALCITE-4606, Search RexCall will be translated to Terms Query, you can see the full explanation here: https://github.com/apache/calcite/pull/2420/files#r647115126.
--
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] zabetak commented on a change in pull request #2659: [CALCITE-4960] Enable unit tests in Elasticsearch Adapter
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2659:
URL: https://github.com/apache/calcite/pull/2659#discussion_r777248770
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/ElasticSearchAdapterTest.java
##########
@@ -476,8 +484,8 @@ public static void setupInstance() throws Exception {
@Test void testInPlan() {
final String[] searches = {
- "query: {'constant_score':{filter:{bool:{should:"
Review comment:
OK, got 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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak closed pull request #2659: [CALCITE-4960] Enable unit tests in Elasticsearch Adapter
Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #2659:
URL: https://github.com/apache/calcite/pull/2659
--
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] zabetak commented on a change in pull request #2659: [CALCITE-4960] Enable unit tests in Elasticsearch Adapter
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2659:
URL: https://github.com/apache/calcite/pull/2659#discussion_r777248842
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/EmbeddedElasticsearchPolicy.java
##########
@@ -197,7 +197,11 @@ RestClient restClient() {
return client;
}
- final RestClient client = RestClient.builder(httpHost()).build();
+ final RestClient client = RestClient.builder(httpHost())
+ .setRequestConfigCallback(requestConfigBuilder -> requestConfigBuilder
+ .setConnectTimeout(60 * 1000) // default 1000
+ .setSocketTimeout(3 * 60 * 1000)) // default 30000
+ .build();
Review comment:
Thanks for the clarification.
--
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] ILuffZhe commented on pull request #2659: [CALCITE-4960] Enable unit tests in Elasticsearch Adapter
Posted by GitBox <gi...@apache.org>.
ILuffZhe commented on pull request #2659:
URL: https://github.com/apache/calcite/pull/2659#issuecomment-1003834731
Thanks for your review, @zabetak !
--
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] zabetak commented on a change in pull request #2659: [CALCITE-4960] Enable unit tests in Elasticsearch Adapter
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2659:
URL: https://github.com/apache/calcite/pull/2659#discussion_r775063037
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/ElasticSearchAdapterTest.java
##########
@@ -462,6 +462,14 @@ public static void setupInstance() throws Exception {
.explainContains(explain);
}
+ /**
+ * Range query is not supported currently.
+ * @see <a href="https://issues.apache.org/jira/browse/CALCITE-4645">[CALCITE-4645]
+ * In Elasticsearch adapter, a range predicate should be translated to a range query</a>
+ * <p>Description about translating search call to range query
+ * {@link org.apache.calcite.adapter.elasticsearch.PredicateAnalyzer.Visitor#canBeTranslatedToTermsQuery(RexCall)}
+ */
+ @Disabled
Review comment:
I think it is better to create an entry in https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/util/Bug.java class and use that value to skip/disable the test.
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/BooleanLogicTest.java
##########
@@ -120,7 +118,6 @@ public static void setupInstance() throws Exception {
assertEmpty("select * from view where num > 42 and num < 42 and num = 42");
assertEmpty("select * from view where num > 42 or num < 42 and num = 42");
assertSingle("select * from view where num > 42 and num < 42 or num = 42");
- assertSingle("select * from view where num > 42 or num < 42 or num = 42");
Review comment:
Instead of removing completely the test can you please skip the line using a (new) reference from https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/util/Bug.java.
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/EmbeddedElasticsearchPolicy.java
##########
@@ -197,7 +197,11 @@ RestClient restClient() {
return client;
}
- final RestClient client = RestClient.builder(httpHost()).build();
+ final RestClient client = RestClient.builder(httpHost())
+ .setRequestConfigCallback(requestConfigBuilder -> requestConfigBuilder
+ .setConnectTimeout(60 * 1000) // default 1000
+ .setSocketTimeout(3 * 60 * 1000)) // default 30000
+ .build();
Review comment:
How did you choose the new values? Did you do any tests?
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/ElasticSearchAdapterTest.java
##########
@@ -476,8 +484,8 @@ public static void setupInstance() throws Exception {
@Test void testInPlan() {
final String[] searches = {
- "query: {'constant_score':{filter:{bool:{should:"
Review comment:
Why did `bool:should` disappeared?
--
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] ILuffZhe commented on a change in pull request #2659: [CALCITE-4960] Enable unit tests in Elasticsearch Adapter
Posted by GitBox <gi...@apache.org>.
ILuffZhe commented on a change in pull request #2659:
URL: https://github.com/apache/calcite/pull/2659#discussion_r775104362
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/EmbeddedElasticsearchPolicy.java
##########
@@ -197,7 +197,11 @@ RestClient restClient() {
return client;
}
- final RestClient client = RestClient.builder(httpHost()).build();
+ final RestClient client = RestClient.builder(httpHost())
+ .setRequestConfigCallback(requestConfigBuilder -> requestConfigBuilder
+ .setConnectTimeout(60 * 1000) // default 1000
+ .setSocketTimeout(3 * 60 * 1000)) // default 30000
+ .build();
Review comment:
(1) I’ve checked some CI tests, where I find tests running time for Elasticsearch:test **ranges from 18s to 60s**, it’s hard to determine a perfect timeout value for RestClient due to the network bandwidth and resources fluctuations. In our own project, we set those two timeout for 60s and 180s based on experience and actual use.
(2) As for timeout tests, I’ve debug this several times by setting socketTimeout to 1, and “1 milliseconds timeout on connection http-outgoing-0”(exception msg changed when you set different timeout) occurred when timeout reached. So, it's working.
--
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] ILuffZhe commented on a change in pull request #2659: [CALCITE-4960] Enable unit tests in Elasticsearch Adapter
Posted by GitBox <gi...@apache.org>.
ILuffZhe commented on a change in pull request #2659:
URL: https://github.com/apache/calcite/pull/2659#discussion_r775104564
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/ElasticSearchAdapterTest.java
##########
@@ -462,6 +462,14 @@ public static void setupInstance() throws Exception {
.explainContains(explain);
}
+ /**
+ * Range query is not supported currently.
+ * @see <a href="https://issues.apache.org/jira/browse/CALCITE-4645">[CALCITE-4645]
+ * In Elasticsearch adapter, a range predicate should be translated to a range query</a>
+ * <p>Description about translating search call to range query
+ * {@link org.apache.calcite.adapter.elasticsearch.PredicateAnalyzer.Visitor#canBeTranslatedToTermsQuery(RexCall)}
+ */
+ @Disabled
Review comment:
Hi, @zabetak .I didn't know such an entry for Bug label before.
I've added those two bugs there, thanks for your reminder!
--
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] ILuffZhe commented on a change in pull request #2659: [CALCITE-4960] Enable unit tests in Elasticsearch Adapter
Posted by GitBox <gi...@apache.org>.
ILuffZhe commented on a change in pull request #2659:
URL: https://github.com/apache/calcite/pull/2659#discussion_r775104572
##########
File path: elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/BooleanLogicTest.java
##########
@@ -120,7 +118,6 @@ public static void setupInstance() throws Exception {
assertEmpty("select * from view where num > 42 and num < 42 and num = 42");
assertEmpty("select * from view where num > 42 or num < 42 and num = 42");
assertSingle("select * from view where num > 42 and num < 42 or num = 42");
- assertSingle("select * from view where num > 42 or num < 42 or num = 42");
Review comment:
Done.
--
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