Refer alias of select fields in group by field #205
Conversation
| @@ -103,8 +103,9 @@ public AggregationBuilder makeGroupAgg(Field field) throws SqlParseException { | |||
| } | |||
| return makeRangeGroup(methodField); | |||
| } else { | |||
| TermsAggregationBuilder termsBuilder = AggregationBuilders.terms(field.getName()).field(field.getName()); | |||
| groupMap.put(field.getName(), new KVValue("KEY", termsBuilder)); | |||
| String termName = (null != field.getAlias()) ? field.getAlias() : field.getName(); | |||
There was a problem hiding this comment.
Suggest to cover the empty string case by using Strings.isNullOrEmpty.
There was a problem hiding this comment.
sure, I'll change to use that.
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (lastAgg == null) { | ||
| if (shadowField == null) { | ||
| for (Field selectField: select.getFields()) { |
There was a problem hiding this comment.
Is it possilbe to combine this forloop with the forloop on L241. For me, the if null check of shadowFiled is a little bit complex.
There was a problem hiding this comment.
I'm not sure whether it was two loops at the beginning, I prefer keep it as is to avoid breaking any logic.
|
Just a heads up, we have major code changes of several feature/bug fixes merged today. Please pull from master branch and make sure your changes can pass |
Thanks for the head up. I just rebased, it could build successfully and passed all the tests. |
*Issue #186
Description of changes:
This could partially solve the problems mentioned in #186
It works if the expression in select fields and group by fields are the same.
To solve the issue completely, we probably need to use scripted metrics aggregation.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.