Support Top/Rare Command In PPL #720

rupal-bq · 2020-08-28T17:40:17Z

Issue #685

Description of changes:

core: added support for rare/top operation in a logical & physical layer
ppl: added AstBuilder logic for rare and top command
updated documentation with examples for using rare and top command

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…re-top

…ch other

…re-top

raymond-lum-zz · 2020-08-28T17:50:51Z

.../src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanDSL.java

@@ -83,6 +84,16 @@ public static LogicalPlan dedupe(
        input, Arrays.asList(fields), allowedDuplication, keepEmpty, consecutive);
  }

+  public static LogicalPlan rareTopN(LogicalPlan input, Boolean rareTopFlag,
+      List<Expression> groupByList, Expression... fields) {
+    return rareTopN(input, rareTopFlag, 10, groupByList, fields);


Some use a constant like DEFAULT_NO_OF_RESULTS and some have 10 hardcoded. Should probably be consistent with the constant.

Used the variable/function to get default value wherever required. hardcoded values are used for the test purpose.

...src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalRareTopN.java

jduo · 2020-08-28T18:34:43Z

...src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalRareTopN.java

+
+  @Override
+  public List<LogicalPlan> getChild() {
+    return Collections.singletonList(child);


In a different method we use ImmutableList.of() here. Let's be consistent.

Replaced ImmutableList.of(child) in RareTopN.java with Collections.singletonList(child)

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

jduo · 2020-08-28T18:46:53Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+     * Get a list of top values.
+     */
+    public List<FieldKey> findTop(HashMap<FieldKey, Integer> map) {
+      PriorityQueue<FieldKey> topQueue = new PriorityQueue<>(new Comparator<FieldKey>() {


findTop() and findRare() have the same implementation, except the comparator is different. We could just have a single find() function that takes in a comparator instead.

replaced with find() function

jduo · 2020-08-28T18:53:04Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+    /**
+     * Get a list of result.
+     */
+    public List<FieldKey> getList(HashMap<FieldKey, Integer> map, PriorityQueue<FieldKey> queue,


This algorithm for building this list is too complicated. I think you can model this more easily with streams.
map.entrySet().stream()
.sort(ValueComparator)
.limit(noOfResults)

Pass in different comparator for rare vs. top.
.

Thanks. Added this in find() function.

jduo · 2020-08-28T18:58:11Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+    /**
+     * Return the Map of field and field value.
+     */
+    public LinkedHashMap<String, ExprValue> fieldKeyMap() {


Usually you don't want to return the specific type and instead return the interface unless the caller really needs to work with LinkedHashMap in particular.

Thanks. Changed return type to Map.

jduo · 2020-08-29T17:54:59Z

core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/tree/RareTopN.java

@@ -0,0 +1,49 @@
+package com.amazon.opendistroforelasticsearch.sql.ast.tree;


Missing the license header.

jduo · 2020-08-29T17:58:01Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+     */
+    public FieldKey(ExprValue value) {
+      this.fieldByValueList = new ArrayList<>();
+      for (Expression fieldExpr : fieldExprList) {


Lets use fieldExprList.stream().map(...).collect(Collectors.toList())

Switched to stream

jduo · 2020-08-29T18:06:28Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+     */
+    public Map<String, ExprValue> fieldKeyMap() {
+      Map<String, ExprValue> map = new LinkedHashMap<>();
+      for (int i = 0; i < fieldExprList.size(); i++) {


Are you using guava? Can you use Streams.zip().collect(Collectors.toMap(...))?

Replaced with Streams.zip()...

jduo · 2020-08-29T18:07:21Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+     */
+    public GroupKey(ExprValue value) {
+      this.groupByValueList = new ArrayList<>();
+      for (Expression groupExpr : groupByExprList) {


Switch to stream

jduo · 2020-08-29T18:07:43Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+    /**
+     * Return the Map of group field and group field value.
+     */
+    public Map<String, ExprValue> groupKeyMap() {


Switch to stream.

This is the same logic and return type as for FieldKey#fieldKeyMap. Can they derive from a common base, or use a generic parameter?

The constructor is also pretty much the same.

Used common class for finding group & field key

jduo · 2020-08-29T18:18:59Z

ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java

+   * Top command.
+   */
+  @Override
+  public UnresolvedPlan visitTopCommand(TopCommandContext ctx) {


Code is the same for these two methods, except differing by the flag. Can they be combined into a helper? If the two context classes have the same methods...

Added helper methods for getting a list of groups & fields. Can't add other arguments in helper since that requires respective command context object

penghuo · 2020-09-01T20:10:50Z

docs/experiment/ppl/cmd/rare.rst

+
+PPL query::
+
+    od> source=accounts | rare age by gender;


how many value for each field will be returned? 10?

10 values per group for each field

could you add this to the doc? I think the max value = 10, right?

Yes, the max is 10. Added in the Description section.

penghuo · 2020-09-01T20:12:32Z

docs/experiment/ppl/cmd/top.rst

+============
+top [N] <field-list> [by-clause]
+
+* N: number of results to return. **Default**: 10


why the rare command doesn't has thsi paramater?

Rare doesn't have N because it's not in the syntax given in this documentation: https://docs.splunk.com/Documentation/Splunk/8.0.2/SearchReference/Rare

limit controls the number of results for rare but support for top-options isn't covered in this PR.
Should I add parameter N for rare?

no need now, just confirm,

penghuo · 2020-09-01T20:16:09Z

ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java

@@ -197,6 +206,56 @@ public UnresolvedPlan visitEvalCommand(EvalCommandContext ctx) {
    );
  }

+  private List<UnresolvedExpression> getGroupByList(ByClauseContext ctx) {


could you also change the stats command to use these genric method?

used getGroupByList in stats command

penghuo · 2020-09-01T20:16:15Z

ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java

+        .collect(Collectors.toList());
+  }
+
+  private List<Field> getFieldList(FieldListContext ctx) {


used getFieldList for dedup command

penghuo · 2020-09-01T20:33:27Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+  @RequiredArgsConstructor
+  public class Group {
+
+    private final Map<Key, List<HashMap<Key, Integer>>> groupListMap = new HashMap<>();


why the value is List instead of just Map?

Thanks for noticing. The list is not required here. I will remove it.
I had list<list<ExprValue>> before I understood every input has values for the entire row and not just for a cell. Looks like I missed this while updating the structure.

removed list

- remove list - use generic getGroupByList function for stats - use generic getFieldList function for dedup

jduo · 2020-09-02T17:39:42Z

core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/tree/RareTopN.java

+public class RareTopN extends UnresolvedPlan {
+
+  private UnresolvedPlan child;
+  private final Boolean rareTopFlag;


Should this be a boolean instead of Boolean? Would an enum with values RARE or TOP be better? I can't tell if setting this to true means rare or top.

Will add enum

replaced with enum

jduo · 2020-09-02T17:41:55Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+  @EqualsAndHashCode.Exclude
+  private Iterator<ExprValue> iterator;
+
+  private static final Integer DEFAULT_NO_OF_RESULTS = 10;


Can this and noOfResults just be primitive ints?

Yes, I can replace it with primitive ints. All other operators had an object of Integer so I just followed the same structure. Should I use primitive instead?

jduo · 2020-09-02T17:43:00Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+
+  @Override
+  public boolean hasNext() {
+    return iterator.hasNext();


Let's add a precondition check here and on next() to verify open() has been called.

Can you elaborate more? Like what should we return if a condition fails? Maybe Null?
I have simply followed the logic of AggregationOperator here. It might not be required since open() is updating an iterator and if open() isn't called then hasNext() will simply return false since there are no elements. And when hasNext() will return false, next() won't be called.

I would add Precondition.checkNotNull(iterator) here. But really depends on what coding style is preferred.

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

penghuo

Thanks for the change.

raymond-lum-zz · 2020-08-28T17:51:12Z

...rc/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanDSL.java

@@ -80,6 +80,17 @@ public static DedupeOperator dedupe(
        input, Arrays.asList(expressions), allowedDuplication, keepEmpty, consecutive);
  }

+  public static RareTopNOperator rareTopN(PhysicalPlan input, Boolean rareTopFlag,
+      List<Expression> groups, Expression... expressions) {
+    return rareTopN(input, rareTopFlag, 10, groups, expressions);


Same comment on constant for 10.

raymond-lum-zz · 2020-08-28T17:55:49Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+    }
+
+    /**
+     * Get a list of top values.


We should use tags like @return and such when doing java docs for return values. Similar comment for @param for parameter. Would generally have this nit across the code.

raymond-lum-zz · 2020-08-28T18:25:57Z

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java

+  }
+
+  /**
+   * Field Key.


This javadoc isn't very useful. Comments should help the developer understand the code and provide value like "what, why, how".

Rupal Mahajan added 11 commits August 17, 2020 14:28

ast - rare and top command

5fe5aa7

fix build failure

4c2000a

fix build failure - test coverage ratio for ppl

2d98f31

Merge branch 'develop' of https://github.com/rupal-bq/sql into ppl/ra…

740a9f0

…re-top

add logical, physical plan for rare & top

016a886

Merge branch 'develop' of https://github.com/rupal-bq/sql into ppl/ra…

574cadb

…re-top

merge rare & top operator since both commands operate identical to ea…

b4aa915

…ch other

add tests

372bf31

update docs

01245a3

Merge branch 'develop' of https://github.com/rupal-bq/sql into ppl/ra…

97cef7f

…re-top

add comments

c41319b

dai-chen added the PPL label Aug 28, 2020

raymond-lum-zz reviewed Aug 28, 2020

View reviewed changes

jduo suggested changes Aug 28, 2020

View reviewed changes

Rupal Mahajan added 2 commits August 28, 2020 16:34

addressing PR comments

c9e4353

fix build error

8582d37

jduo reviewed Aug 29, 2020

View reviewed changes

jduo approved these changes Aug 29, 2020

View reviewed changes

address PR comments

a802b55

rupal-bq marked this pull request as ready for review August 31, 2020 22:06

penghuo reviewed Sep 1, 2020

View reviewed changes

Rupal Mahajan added 3 commits September 1, 2020 15:35

address PR comments:

0fe9d9b

- remove list - use generic getGroupByList function for stats - use generic getFieldList function for dedup

use interface instead of specific type for input of find()

8c33006

update doc

1612815

jduo reviewed Sep 2, 2020

View reviewed changes

...c/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/RareTopNOperator.java Show resolved Hide resolved

replace rareTopFlag with enum

77d6bf3

penghuo approved these changes Sep 4, 2020

View reviewed changes

raymond-lum-zz approved these changes Sep 8, 2020

View reviewed changes

penghuo merged commit 94f2d32 into opendistro-for-elasticsearch:develop Sep 8, 2020

		@@ -0,0 +1,49 @@
		package com.amazon.opendistroforelasticsearch.sql.ast.tree;

Support Top/Rare Command In PPL #720

Support Top/Rare Command In PPL #720

Conversation

rupal-bq commented Aug 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jduo Aug 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penghuo Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rupal-bq Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penghuo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jduo Aug 29, 2020 •

edited

Loading

penghuo Sep 1, 2020 •

edited

Loading

rupal-bq Sep 1, 2020 •

edited

Loading