asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "James Fang (Code Review)" <>
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:

Commit Message:

Line 7: WIP: stddev
> Follow the commit message template. So it should be something like:
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
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
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
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"
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'
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'
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
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 
File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/

Line 1406:         addPrivateFunction(SERIAL_LOCAL_SQL_STDDEV, LocalAvgTypeComputer.INSTANCE,
> should be LocalSingleVarStatisticsTypeComputer.INSTANCE

Line 1407:         addPrivateFunction(SERIAL_INTERMEDIATE_SQL_STDDEV, LocalAvgTypeComputer.INSTANCE,
> should be LocalSingleVarStatisticsTypeComputer.INSTANCE
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/serializable/std/

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

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

Line 303:                 int offset5 = ARecordSerializerDeserializer.getFieldOffsetById(serBytes,
> should be "offset3"
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/

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

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"?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia709669a9d20358f11ad28f453ae8ad8551f6334
Gerrit-PatchSet: 4
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: James Fang <>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Dmitry Lychagin <>
Gerrit-Reviewer: James Fang <>
Gerrit-Reviewer: Jenkins <>
Gerrit-HasComments: Yes

View raw message