asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ali Alsuliman (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: [ASTERIXDB-2466][FUN] Implement window functions
Date Sun, 28 Oct 2018 01:21:36 GMT
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 <dmitry.lychagin@couchbase.com>
Gerrit-Reviewer: Ali Alsuliman <ali.al.solaiman@gmail.com>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: James Fang <jfang003@ucr.edu>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Taewoo Kim <wangsaeu@gmail.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message