-
Notifications
You must be signed in to change notification settings - Fork 186
Support Top/Rare Command In PPL #720
Changes from 11 commits
5fe5aa7
4c2000a
2d98f31
740a9f0
016a886
574cadb
b4aa915
372bf31
01245a3
97cef7f
c41319b
c9e4353
8582d37
a802b55
0fe9d9b
8c33006
1612815
77d6bf3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package com.amazon.opendistroforelasticsearch.sql.ast.tree; | ||
|
||
import com.amazon.opendistroforelasticsearch.sql.ast.AbstractNodeVisitor; | ||
import com.amazon.opendistroforelasticsearch.sql.ast.expression.Argument; | ||
import com.amazon.opendistroforelasticsearch.sql.ast.expression.Field; | ||
import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression; | ||
import com.google.common.collect.ImmutableList; | ||
import java.util.List; | ||
import lombok.AllArgsConstructor; | ||
import lombok.EqualsAndHashCode; | ||
import lombok.Getter; | ||
import lombok.RequiredArgsConstructor; | ||
import lombok.Setter; | ||
import lombok.ToString; | ||
|
||
/** | ||
* AST node represent RareTopN operation. | ||
*/ | ||
@Getter | ||
@Setter | ||
@ToString | ||
@EqualsAndHashCode(callSuper = false) | ||
@RequiredArgsConstructor | ||
@AllArgsConstructor | ||
public class RareTopN extends UnresolvedPlan { | ||
|
||
private UnresolvedPlan child; | ||
private final Boolean rareTopFlag; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will add enum There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. replaced with enum |
||
private final List<Argument> noOfResults; | ||
private final List<Field> fields; | ||
private final List<UnresolvedExpression> groupExprList; | ||
|
||
@Override | ||
public RareTopN attach(UnresolvedPlan child) { | ||
this.child = child; | ||
return this; | ||
} | ||
|
||
@Override | ||
public List<UnresolvedPlan> getChild() { | ||
return ImmutableList.of(this.child); | ||
} | ||
|
||
@Override | ||
public <T, C> T accept(AbstractNodeVisitor<T, C> nodeVisitor, C context) { | ||
return nodeVisitor.visitRareTopN(this, context); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,7 @@ | |
*/ | ||
@UtilityClass | ||
public class LogicalPlanDSL { | ||
|
||
public static LogicalPlan aggregation( | ||
LogicalPlan input, List<Aggregator> aggregatorList, List<Expression> groupByList) { | ||
return new LogicalAggregation(input, aggregatorList, groupByList); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some use a constant like DEFAULT_NO_OF_RESULTS and some have 10 hardcoded. Should probably be consistent with the constant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Used the variable/function to get default value wherever required. hardcoded values are used for the test purpose. |
||
} | ||
|
||
public static LogicalPlan rareTopN(LogicalPlan input, boolean rareTopFlag, int noOfResults, | ||
List<Expression> groupByList, Expression... fields) { | ||
return new LogicalRareTopN(input, rareTopFlag, noOfResults, Arrays.asList(fields), groupByList); | ||
} | ||
|
||
@SafeVarargs | ||
public LogicalPlan values(List<LiteralExpression>... values) { | ||
return new LogicalValues(Arrays.asList(values)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package com.amazon.opendistroforelasticsearch.sql.planner.logical; | ||
rupal-bq marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
import com.amazon.opendistroforelasticsearch.sql.expression.Expression; | ||
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import lombok.EqualsAndHashCode; | ||
import lombok.Getter; | ||
import lombok.RequiredArgsConstructor; | ||
import lombok.ToString; | ||
|
||
/** | ||
* Logical Rare and TopN Plan. | ||
*/ | ||
@Getter | ||
@ToString | ||
@EqualsAndHashCode(callSuper = false) | ||
@RequiredArgsConstructor | ||
public class LogicalRareTopN extends LogicalPlan { | ||
|
||
private final LogicalPlan child; | ||
private final Boolean rareTopFlag; | ||
private final Integer noOfResults; | ||
private final List<Expression> fieldList; | ||
private final List<Expression> groupByList; | ||
|
||
@Override | ||
public List<LogicalPlan> getChild() { | ||
return Collections.singletonList(child); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a different method we use ImmutableList.of() here. Let's be consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaced |
||
} | ||
|
||
@Override | ||
public <R, C> R accept(LogicalPlanNodeVisitor<R, C> visitor, C context) { | ||
return visitor.visitRareTopN(this, context); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment on constant for 10. |
||
} | ||
|
||
public static RareTopNOperator rareTopN(PhysicalPlan input, Boolean rareTopFlag, int noOfResults, | ||
List<Expression> groups, Expression... expressions) { | ||
return new RareTopNOperator(input, rareTopFlag, noOfResults, Arrays.asList(expressions), | ||
groups); | ||
} | ||
|
||
@SafeVarargs | ||
public ValuesOperator values(List<LiteralExpression>... values) { | ||
return new ValuesOperator(Arrays.asList(values)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the license header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added