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).
c = convertInput(a)
result = compute(c)
convertOutput(result)
The only way to figure out a type of an aggregate function that runtime is
going to use is to call `Accumulator::returnType`, but since it returns a type
without scale, the scale can be lost during serialisation.
##########
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 is to call `Accumulator::returnType`, but since it returns a type
without scale, the scale can be lost during serialisation.
--
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]
