asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "James Fang (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: [ASTERIXDB-2459][FUN] Add sttdev() aggregate function
Date Wed, 10 Oct 2018 00:51:59 GMT
James Fang has posted comments on this change.

Change subject: [ASTERIXDB-2459][FUN] Add sttdev() aggregate function
......................................................................


Patch Set 4:

(18 comments)

https://asterix-gerrit.ics.uci.edu/#/c/2990/4//COMMIT_MSG
Commit Message:

Line 7: WIP: stddev
> Follow the commit message template. So it should be something like:
Done


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate-sql/serial_stddev_double_null/serial_stddev_double_null.2.update.sqlpp
File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate-sql/serial_stddev_double_null/serial_stddev_double_null.2.update.sqlpp:

Line 25: select element {'id':1,'gid':1,'val':double(5.32)};
> It looks like the intent was to test cases when valplus is null, but in thi
Done


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/agg_null_rec/agg_null_rec.3.query.sqlpp
File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/agg_null_rec/agg_null_rec.3.query.sqlpp:

Line 43: )), 'stddev':test.coll_stddev((
> should be "strict_stddev". "coll_" prefix still works but is deprecated
Done


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/agg_null_rec_1/agg_null_rec_1.3.query.sqlpp
File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/agg_null_rec_1/agg_null_rec_1.3.query.sqlpp:

Line 56:     select element i.val
> should it be i.val (like in avg/sum above) or i.valplus (like in  min/max a
It does not mention which one, and all other aggregates have the similar cases. I choose to
use val to match avg/sum


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/agg_number_rec/agg_number_rec.3.query.sqlpp
File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/agg_number_rec/agg_number_rec.3.query.sqlpp:

Line 43: )),'stddev':test.coll_stddev((
> should be "strict_stddev"
Done


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/stddev_float_null/stddev_float_nu.1.ddl.sqlpp
File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/stddev_float_null/stddev_float_nu.1.ddl.sqlpp:

Line 1: /*
> type in the name of this file. should be '_null', not '_nu'
Done


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/stddev_float_null/stddev_float_nu.3.query.sqlpp
File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/aggregate/stddev_float_null/stddev_float_nu.3.query.sqlpp:

Line 1: /*
> type in the name of this file. should be '_null', not '_nu'
Done


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate-sql/serial_stddev_float/serial_stddev_float.1.adm
File asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate-sql/serial_stddev_float/serial_stddev_float.1.adm:

Line 1: { "gid": 1, "stddev": 00.9574271077563381 }
> why two 0s before the decimal point?
Fixed with only one 0


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate-sql/sum_int8_null/sum_int8_null.1.adm
File asterixdb/asterix-app/src/test/resources/runtimets/results/aggregate-sql/sum_int8_null/sum_int8_null.1.adm:

Line 1: -112
> this seems to be wrong. please file an issue for this and meanwhile change 
Done


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java
File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java:

Line 1406:         addPrivateFunction(SERIAL_LOCAL_SQL_STDDEV, LocalAvgTypeComputer.INSTANCE,
true);
> should be LocalSingleVarStatisticsTypeComputer.INSTANCE
Done


Line 1407:         addPrivateFunction(SERIAL_INTERMEDIATE_SQL_STDDEV, LocalAvgTypeComputer.INSTANCE,
true);
> should be LocalSingleVarStatisticsTypeComputer.INSTANCE
Done


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/serializable/std/AbstractSerializableSingleVariableStatisticsAggregateFunction.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/serializable/std/AbstractSerializableSingleVariableStatisticsAggregateFunction.java:

Line 173:         moments m = new moments(m1, m2, count);
> let's not create a new object for each tuple. We need to figure out a way t
Fixed with the new class SingleVarFunctionsUtil


Line 194:             case TINYINT: {
> Remove { } from case blocks to fix this SonarQube complaint
Done


Line 279:         moments m = new moments(m1, m2, count);
> same comment. avoid object creation
Same as above


Line 303:                 int offset5 = ARecordSerializerDeserializer.getFieldOffsetById(serBytes,
offset, COUNT_FIELD_ID,
> should be "offset3"
Done


https://asterix-gerrit.ics.uci.edu/#/c/2990/4/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/AbstractSingleVarStatisticsAggregateFunction.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/AbstractSingleVarStatisticsAggregateFunction.java:

Line 96:                 new IAType[] { BuiltinType.ADOUBLE, BuiltinType.ADOUBLE, BuiltinType.AINT64
}, false);
> LocalSingleVarStatisticsTypeComputer says that "count"'s type in AINT32, bu
Done


Line 121:         double delta = val - m1;
> Need to figure out how to reuse this code together with AbstractSerializabl
Fixed by making a new class called SingleVarFunctionsUtil


PS4, Line 258: offset5
> should be "offset3", not "5"?
Done


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/2990
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia709669a9d20358f11ad28f453ae8ad8551f6334
Gerrit-PatchSet: 4
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: James Fang <jfang003@ucr.edu>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Dmitry Lychagin <dmitry.lychagin@couchbase.com>
Gerrit-Reviewer: James Fang <jfang003@ucr.edu>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-HasComments: Yes

Mime
View raw message