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 2020/07/30 09:27:52 UTC

[GitHub] [calcite] yuqi1129 opened a new pull request #2075: [CALCITE-4130] Aggregation function in order by clause will throw Exception

yuqi1129 opened a new pull request #2075:
URL: https://github.com/apache/calcite/pull/2075


   …e will throw Exception


----------------------------------------------------------------
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] yuqi1129 closed pull request #2075: [CALCITE-4130] Aggregation function in order by clause will throw Exception

Posted by GitBox <gi...@apache.org>.
yuqi1129 closed pull request #2075:
URL: https://github.com/apache/calcite/pull/2075


   


----------------------------------------------------------------
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] zinking commented on a change in pull request #2075: [CALCITE-4130] Aggregation function in order by clause will throw Exception

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



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

Posted by GitBox <gi...@apache.org>.
yuqi1129 commented on pull request #2075:
URL: https://github.com/apache/calcite/pull/2075#issuecomment-665385825






----------------------------------------------------------------
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] julianhyde commented on pull request #2075: [CALCITE-4130] Aggregation function in order by clause will throw Exception

Posted by GitBox <gi...@apache.org>.
julianhyde commented on pull request #2075:
URL: https://github.com/apache/calcite/pull/2075#issuecomment-665223274


   Tweak the commit message: "In Elasticsearch adapter, aggregate function in ORDER BY clause throws Exception".


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