[GitHub] [calcite] zinking commented on a change in pull request #2075: [CALCITE-4130] Aggregation function in order by clause will throw Exception

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

zinking commented on a change in pull request #2075:
URL: https://github.com/apache/calcite/pull/2075#discussion_r461497001



##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchTable.java
##########
@@ -212,6 +217,14 @@ String scriptedFieldPrefix() {
     // construct nested aggregations node(s)
     ObjectNode parent = query.with(AGGREGATIONS);
     for (String name: orderedGroupBy) {
+
+      //select id, count(id) from test group by id order by count(id)
+      //id should not be group again
+      if (null != getFieldTypeFromName(name, aggregations)
+          && transport.mapping.mapping().get(name) == null) {

Review comment:
       why `null` is on right side ?

##########
File path: elasticsearch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchTable.java
##########
@@ -309,6 +322,36 @@ String scriptedFieldPrefix() {
     return Linq4j.asEnumerable(hits.hits()).select(getter);
   }
 
+  private String getFieldTypeFromName(String key, List<Map.Entry<String, String>> aggregations)
+      throws JsonProcessingException {
+    ElasticsearchMapping.Datatype type = transport.mapping.mapping().get(key);
+    if (null != type) {
+      return type.name();
+    }
+
+    Optional<Map.Entry<String, String>> entryOptional = aggregations.stream()
+        .filter(agg -> agg.getKey().equals(key))
+        .findAny();
+    if (!entryOptional.isPresent()) {

Review comment:
       what's the point of using Optional, if `null` is required anyways?  
   maybe just use find 




----------------------------------------------------------------
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