ygerzhedovich commented on code in PR #4522: URL: https://github.com/apache/ignite-3/pull/4522#discussion_r1795536331
########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/agg/AccumulatorWrapper.java: ########## @@ -17,12 +17,25 @@ package org.apache.ignite.internal.sql.engine.exec.exp.agg; +import org.jetbrains.annotations.Nullable; + /** * AccumulatorWrapper interface. - * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859 Review Comment: can you elaborate a little bit more what the purpose of the interface? ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/agg/Accumulators.java: ########## @@ -400,25 +447,34 @@ public RelDataType returnType(IgniteTypeFactory typeFactory) { } } - private static class LongCount implements Accumulator { + /** {@code COUNT(LONG)} accumulator.. */ + public static class LongCount implements Accumulator { public static final Supplier<Accumulator> FACTORY = LongCount::new; - private long cnt; - /** {@inheritDoc} */ @Override - public void add(Object... args) { + public void add(AccumulatorsState state, Object... args) { assert nullOrEmpty(args) || args.length == 1; if (nullOrEmpty(args) || args[0] != null) { - cnt++; + Long cnt = (Long) state.get(); Review Comment: maybe better to introduce a separate class-wrapper for long primitive type, to avoid boxing/unboxing every time ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/agg/Accumulators.java: ########## @@ -541,27 +619,35 @@ public RelDataType returnType(IgniteTypeFactory typeFactory) { } } - private static class DecimalSumEmptyIsZero implements Accumulator { + /** SUM(DECIMAL) accumulator. */ + public static class DecimalSumEmptyIsZero implements Accumulator { public static final Supplier<Accumulator> FACTORY = DecimalSumEmptyIsZero::new; - private BigDecimal sum = BigDecimal.ZERO; - /** {@inheritDoc} */ @Override - public void add(Object... args) { + public void add(AccumulatorsState state, Object... args) { BigDecimal in = (BigDecimal) args[0]; if (in == null) { return; } - sum = sum.add(in); + BigDecimal sum = (BigDecimal) state.get(); + if (sum == null) { + state.set(in); + } else { + state.set(sum.add(in)); + } } /** {@inheritDoc} */ @Override - public Object end() { - return sum; + public void end(AccumulatorsState state, AccumulatorsState result) { + if (!state.hasValue()) { Review Comment: can we always put ZERO during creation of the accumulator and reduce all code that check have we null or not? ########## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/agg/Accumulators.java: ########## @@ -434,62 +490,76 @@ public RelDataType returnType(IgniteTypeFactory typeFactory) { } } - private static class Sum implements Accumulator { + /** Wraps another sum accumulator and returns {@code null} if there was updates. */ + public static class Sum implements Accumulator { private Accumulator acc; - private boolean empty = true; - public Sum(Accumulator acc) { this.acc = acc; } /** {@inheritDoc} */ - @Override public void add(Object... args) { + @Override + public void add(AccumulatorsState state, Object... args) { if (args[0] == null) { return; } - empty = false; - acc.add(args[0]); + acc.add(state, args); } /** {@inheritDoc} */ - @Override public Object end() { - return empty ? null : acc.end(); + @Override + public void end(AccumulatorsState state, AccumulatorsState result) { + if (!state.hasValue()) { + result.set(null); + } else { + result.set(state.get()); Review Comment: ```suggestion result.set(state.get()); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
