Dmitry Lychagin has posted comments on this change. Change subject: [ASTERIXDB-2466][FUN] Implement window functions ......................................................................
Patch Set 3: (8 comments) Ali, Taewoo, I address your comments. Please take a look. Taewoo, 1. The materialization flag makes operator materialize the partition. It's required by some functions because they need to know the number of rows in each partition (percentile_rank, for example). There's one extra frame used for that. 2. I've removed function definition from misc_01 testcases https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/ExtractWindowExpressionsRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/ExtractWindowExpressionsRule.java: PS2, Line 89: > We could share code if you like. We could redefine this method: Yes, will do https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/SqlppCloneAndSubstituteVariablesVisitor.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/SqlppCloneAndSubstituteVariablesVisitor.java: PS2, Line 428: newWinExpr.addHints(winExpr.getHint > Should we copy also the hints variable? Yes, will do 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 56: private long rank; > Do we use this "dense" variable? Yes, it's used in computeRank() 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: PS2, Line 51: > Should we give it a good name? Yes, will do. 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: PS2, Line 41: > Good name maybe? Yes, will do 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/visitors/FDsAndEquivClassesVisitor.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/FDsAndEquivClassesVisitor.java: PS2, Line 445: > Should we propagate the FD and EQ classes? or is that not the case here? If yes, will do https://asterix-gerrit.ics.uci.edu/#/c/3002/2/hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/aggrun/AbstractWindowPushRuntime.java File hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/aggrun/AbstractWindowPushRuntime.java: PS2, Line 122: copyFrame.resize(buffer.capacity()); > This will reallocate the frame and copy the existing data along with it. Do ok https://asterix-gerrit.ics.uci.edu/#/c/3002/2/hyracks-fullstack/hyracks/hyracks-dataflow-common/src/main/java/org/apache/hyracks/dataflow/common/io/RunFileWriter.java File hyracks-fullstack/hyracks/hyracks-dataflow-common/src/main/java/org/apache/hyracks/dataflow/common/io/RunFileWriter.java: PS2, Line 52: { > Should we remove the exception? Yes, will do -- 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: 3 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: Dmitry Lychagin <[email protected]> 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
