Taewoo Kim has posted comments on this change. Change subject: [ASTERIXDB-2466][FUN] Implement window functions ......................................................................
Patch Set 2: (12 comments) Ali already gave good comments. I have some questions. (1) What does "materialization" do for the window operator? When calculating the required capacity, I saw that part. (2) I think we need to add more run time test cases without using "function". (3) Generally, I think it would be nice if we could add a brief explanation at the beginning of each class. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/AbstractRankRunningAggregateEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/AbstractRankRunningAggregateEvaluator.java: Line 34: public abstract class AbstractRankRunningAggregateEvaluator implements IWindowAggregateEvaluator { I think it would be better to put a brief class comment. Line 56: AbstractRankRunningAggregateEvaluator(IScalarEvaluator[] args, boolean dense, SourceLocation sourceLoc) { Do we use this "dense" variable? https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/DenseRankRunningAggregateDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/DenseRankRunningAggregateDescriptor.java: Line 33: public class DenseRankRunningAggregateDescriptor extends AbstractRunningAggregateFunctionDynamicDescriptor { I think it would be better to put a brief class comment. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/NtileRunningAggregateDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/NtileRunningAggregateDescriptor.java: Line 32: public class NtileRunningAggregateDescriptor extends AbstractRunningAggregateFunctionDynamicDescriptor { I think it would be better to put a brief class comment. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/NtileRunningAggregateEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/NtileRunningAggregateEvaluator.java: Line 37: public class NtileRunningAggregateEvaluator implements IWindowAggregateEvaluator { I think it would be better to put a brief class comment. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/PercentRankRunningAggregateDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/PercentRankRunningAggregateDescriptor.java: Line 33: public class PercentRankRunningAggregateDescriptor extends AbstractRunningAggregateFunctionDynamicDescriptor { I think it would be better to put a brief class comment. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/PercentRankRunningAggregateEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/PercentRankRunningAggregateEvaluator.java: Line 33: class PercentRankRunningAggregateEvaluator extends AbstractRankRunningAggregateEvaluator { I think it would be better to put a brief class comment. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/RankRunningAggregateDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/RankRunningAggregateDescriptor.java: Line 33: public class RankRunningAggregateDescriptor extends AbstractRunningAggregateFunctionDynamicDescriptor { I think it would be better to put a brief class comment. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/RankRunningAggregateEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/RankRunningAggregateEvaluator.java: Line 33: class RankRunningAggregateEvaluator extends AbstractRankRunningAggregateEvaluator { I think it would be better to put a brief class comment. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/RowNumberRunningAggregateDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/RowNumberRunningAggregateDescriptor.java: Line 31: public class RowNumberRunningAggregateDescriptor extends AbstractRunningAggregateFunctionDynamicDescriptor { I think it would be better to put a brief class comment. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/RowNumberRunningAggregateEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/RowNumberRunningAggregateEvaluator.java: Line 33: class RowNumberRunningAggregateEvaluator implements IWindowAggregateEvaluator { I think it would be better to put a brief class comment. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/WindowOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/WindowOperator.java: Line 37: public class WindowOperator extends AbstractAssignOperator { I think it would be better to put a brief class comment. -- To view, visit https://asterix-gerrit.ics.uci.edu/3002 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia28af8773cb11049c38d440c51b9c3cd1ed2bab4 Gerrit-PatchSet: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: James Fang <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
