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 <jfang...@ucr.edu> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <dmitry.lycha...@couchbase.com> Gerrit-Reviewer: James Fang <jfang...@ucr.edu> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-HasComments: Yes
