lowka commented on code in PR #3413:
URL: https://github.com/apache/ignite-3/pull/3413#discussion_r1535067252
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/PlanUtils.java:
##########
@@ -127,9 +127,17 @@ private static void addAccumulatorFields(IgniteTypeFactory
typeFactory, List<Agg
for (int i = 0; i < aggregateCalls.size(); i++) {
AggregateCall call = aggregateCalls.get(i);
-
Accumulator acc = accumulators.accumulatorFactory(call).get();
- RelDataType fieldType = acc.returnType(typeFactory);
+ RelDataType fieldType;
+ // For a decimal type Accumulator::returnType returns a type with
default precision and scale,
+ // that can cause precision loss when a tuple is sent over the
wire by an exchanger/outbox.
+ // Outbox uses its input type as wire format, so if a scale is 0,
then the scale is lost
+ // (see Outbox::sendBatch -> RowHandler::toBinaryTuple ->
BinaryTupleBuilder::appendDecimalNotNull).
Review Comment:
Because implementation of `Accumulator` is precision/scale independent and
it relies on post-computation type conversion to adjust its result. This keeps
accumulator logic simple and allows to reuse existing type conversion functions
defined by runtime (so type conversion work the same everywhere).
The only way to figure out a type of an aggregate function that runtime is
going to use (so we can provide matching MAP / REDUCE with matching types) is
to call `Accumulator::returnType`, but since it returns a type without default
precision and scale, and default scale is 0, the scale can be lost during
serialisation.
There is no output conversion on MAP phase, and there is no input conversion
on REDUCE phase, so one needs to preserve correct scale.
--
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]
