ILuffZhe commented on a change in pull request #2657:
URL: https://github.com/apache/calcite/pull/2657#discussion_r778493504
##########
File path:
elasticsearch/src/test/java/org/apache/calcite/adapter/elasticsearch/AggregationTest.java
##########
@@ -400,6 +400,21 @@ public static void setupInstance() throws Exception {
"aggregations:{'v1.max.field': 'val1'",
"'v2.min.field': 'val2'}"))
.returnsUnordered("v1=7; v2=5");
+ }
+
+ @Test void testNullsSort() {
Review comment:
Hi, @NobiGo , @zabetak , I need a little advice on this patch.
I'm gonna to add some unit tests for NULLS SORT, however,
ElasticSearchAdapterTest use zips-mini.json as its data, which doesn't contain
any null values for each column, so adding or deleting data would cause a lot
code changes. I've thought about two ways:
- add test cases in ElasticSearchAdapterTest without changing
zips-mini.json, only check the generated Elasticsearch Scripts
- add a new class for NULLS SORT, kinda of redundant since there are several
test classes in this module(if any new features cannot be testified based on
the existing tests, always adding a class might be unacceptable)
What do you think?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
