Ali Alsuliman has posted comments on this change. Change subject: [ASTERIXDB-2466][FUN] Implement window functions ......................................................................
Patch Set 2: (15 comments) 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: hasComplexExpressions We could share code if you like. We could redefine this method: hasComplexExpressions(List<T> exprList, Function<T, ILogicalExpression> expressionAccessor); Then, it can be called like: hasComplexExpressions(exprList, Mutable::getValue); The same thing could be done, I think, for extractComplexExpressions(). I think after doing this, we can actually combine those rules that extract expressions in one rule. P.S. SweepIllegalNonfunctionalFunctions extending AbstractExtractExprRule is by mistake, I think, or became obsolete. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-app/src/test/resources/optimizerts/queries/window/window_01.sqlpp File asterixdb/asterix-app/src/test/resources/optimizerts/queries/window/window_01.sqlpp: PS2, Line 58: q1_mixed(2, 2, 2) Should we add few more test cases: 1. window function without partition clause. 2. with group by clause 3. combination of the above. 4. would be nice to have an order-by clause at the end also. It would be nice to have them also in the runtime if the result can be made deterministic. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/window/ntile_01/ntile_01.1.ddl.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/window/ntile_01/ntile_01.1.ddl.sqlpp: PS2, Line 54: ntile(D) Should we add a test case with non-integer variable/arg? The result should be a failure of course https://asterix-gerrit.ics.uci.edu/#/c/3002/2/asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties File asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties: PS2, Line 170: 1095 = Expected function call Did we include a mapping for this error? 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: return new Pair<>(newWinExpr, env); Should we copy also the hints variable? 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: m Should we give it a good name? PS2, Line 59: v Same here? PS2, Line 61: n Same here? 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: m Good name maybe? PS2, Line 43: v Same here? 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: visitWindowOperator( Should we propagate the FD and EQ classes? or is that not the case here? If I'm not mistaken, EnforceStructuralPropertiesRule (besides some other operators) uses FD and EQ classes when it's trying to determine if properties are satisfied and doing some normalization based on the FD and EQ classes. https://asterix-gerrit.ics.uci.edu/#/c/3002/2/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/WindowPOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/WindowPOperator.java: PS2, Line 81: op.getExecutionMode() What would happen if the exec. mode was local? Can that happen? One way I could see this happen is when the window operator ends up in a nested subplan. Not sure if that's OK. PS2, Line 86: List<OrderColumn> lopColumns = new ArrayList<>(); What would happen if partition columns and sort columns are the same but one is ASC and the other is DESC? I know the sort columns are usually different from the partition columns, but was just a thought that came to mind. 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.ensureFrameSize(buffer.capacity()) This will reallocate the frame and copy the existing data along with it. Do we want to keep the old data on purpose? if not, probably we should use resize. 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: throws HyracksDataException Should we remove the exception? -- 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
