Fix long decimal partial aggregation below join #21083

findepi · 2024-03-14T15:49:57Z

findepi · 2024-03-14T15:50:44Z

according to @lukasz-stec 's analysis, this may be fixing some bug related to partial aggregations, but i don't know how to reproduce that bug.

lukasz-stec

lgtm % comments

lukasz-stec · 2024-03-14T16:19:34Z

.../java/io/trino/operator/aggregation/state/LongDecimalWithOverflowAndLongStateSerializer.java

-        Slice slice = variableWidthBlock.getRawSlice();
-        int sliceOffset = variableWidthBlock.getRawSliceOffset(index);
-        int sliceLength = variableWidthBlock.getSliceLength(index);
+        Slice slice = VARBINARY.getSlice(block, index);


The allocates Slice instance per position and @raunaqmorarka changed that via #18868.
If we want be closer the original performance, you could get access to the raw slice as

VariableWidthBlock valueBlock = (VariableWidthBlock) block.getUnderlyingValueBlock(); Slice slice = valueBlock.getRawSlice();

instead of VariableWidthBlock variableWidthBlock = (VariableWidthBlock) block;
plus get the right index
index = block.getUnderlyingValuePosition(index)

block.getUnderlyingValueBlock and block.getUnderlyingValuePosition can still be virtual calls so there is potential regression here anyway.

I like this solution. I also would not wory about the object allocation.

There was pretty clear performance impact of object allocation here #18868 (comment)
We should at least benchmark it before deciding either way.

Honestly, if allocating a single Slice (which is basically a record) is 10% of CPU, I'd expect that benchmark is broken given how much other stuff is happening in an aggregation query.

Anyway, if you actually care, my initial suggestion was:

index = block.getUnderlyingValuePosition(index); block = block.getUnderlyingValueBlock();

block.getUnderlyingValueBlock and block.getUnderlyingValuePosition can still be virtual calls so there is potential regression here anyway.

good point.

lukasz-stec · 2024-03-14T16:20:10Z

...rc/main/java/io/trino/operator/aggregation/state/LongDecimalWithOverflowStateSerializer.java

-        int sliceOffset = variableWidthBlock.getRawSliceOffset(index);
-        int sliceLength = variableWidthBlock.getSliceLength(index);
-
+        Slice slice = VARBINARY.getSlice(block, index);


the same comment about Slice allocation

lukasz-stec · 2024-03-14T16:27:07Z

.../java/io/trino/operator/aggregation/state/LongDecimalWithOverflowAndLongStateSerializer.java

-        Slice slice = variableWidthBlock.getRawSlice();
-        int sliceOffset = variableWidthBlock.getRawSliceOffset(index);
-        int sliceLength = variableWidthBlock.getSliceLength(index);
+        Slice slice = VARBINARY.getSlice(block, index);


As this may be hard to repo using query I would at least add a unit test the deserialize works also for Dictionary and RLE blocks

yeah, i don't know what the contract is for state serializers, that's why i didn't add any test

do we have an example I should follow?

dain · 2024-03-14T17:27:04Z

.../java/io/trino/operator/aggregation/state/LongDecimalWithOverflowAndLongStateSerializer.java

-        Slice slice = variableWidthBlock.getRawSlice();
-        int sliceOffset = variableWidthBlock.getRawSliceOffset(index);
-        int sliceLength = variableWidthBlock.getSliceLength(index);
+        Slice slice = VARBINARY.getSlice(block, index);


I like this solution. I also would not wory about the object allocation.

sopel39

There is also:

LongDecimalWithOverflowStateSerializer
@lukasz-stec comments

Alternative approach is to handle dictionaries in StateCompiler#generateDeserialize as it would prevent polymorphic calls and extra indirection

findepi · 2024-03-14T20:23:18Z

@lukasz-stec can you help me with testing this?
would especially be good to have query-based regression that that uncovers the necessity for state serializers to handle dict blocks.

lukasz-stec · 2024-03-15T08:49:45Z

@lukasz-stec can you help me with testing this? would especially be good to have query-based regression that that uncovers the necessity for state serializers to handle dict blocks.

@findepi I don't know how to repro this using a query. It may be imposiible at the current state. It requires partial and final or intermediate aggregation to be separated by operation that can produce dictionary blocks for aggregation state. Join is one potential such operation.

Here are unit tests that cover this though.
Test_dict_and_rle_blocks_deserialization.patch

findepi · 2024-03-15T09:03:48Z

There is also:

LongDecimalWithOverflowStateSerializer

@sopel39 this is the first class i fixed. is there some other state serializer class i missed?
i searched for (VariableWidthBlock) in remaining state serializers, but didn't find any

findepi · 2024-03-15T09:04:05Z

I don't know how to repro this using a query. It may be imposiible at the current state

i was able -- #21099

findepi · 2024-03-15T09:04:56Z

Here are unit tests that cover this though.
Test_dict_and_rle_blocks_deserialization.patch

thanks!

findepi · 2024-03-15T09:31:39Z

i added a unit test from @lukasz-stec , thank you
i added a query-based regression test (from #21099 (comment))

There is open discussion how to write the code the most performant way and there seems to be disagreement how this should look like. I propose we merge the code, since it's an obvious improvement over current (broken) state.
Alternatively we do not merge this code, but then I would like you guys, @sopel39 @raunaqmorarka @lukasz-stec , take this over.

PTAL

sopel39 · 2024-03-15T09:31:47Z

@findepi let's use @dain approach:
#21083 (comment)
if you don't plan to handle dictionaries in StateCompiler#generateDeserialize

findepi · 2024-03-15T09:33:12Z

@lukasz-stec had concerns. @lukasz-stec will you be OK with that approach?

Co-authored-by: Dain Sundstrom <[email protected]> Co-authored-by: Lukasz Stec <[email protected]>

lukasz-stec · 2024-03-15T09:37:56Z

@lukasz-stec had concerns. @lukasz-stec will you be OK with that approach?

Yes, the only concern was a potential virtual call and a) it is not that bad, b) it will happen only in rare conditions if at all as it needs all three block types to be there (value, dict, RLE)

- use `Long.BYTES * n` offsets as in the serializer code (easier to correlate the two) - use same control flow pattern in the two similar classes

findepi · 2024-03-15T09:39:52Z

updated, PTAL

sopel39

awesome! thanks

mosabua · 2024-03-21T03:09:47Z

Since this fixes an issue I added a release notes entry

findepi requested review from dain and sopel39 March 14, 2024 15:49

cla-bot bot added the cla-signed label Mar 14, 2024

lukasz-stec approved these changes Mar 14, 2024

View reviewed changes

dain approved these changes Mar 14, 2024

View reviewed changes

sopel39 requested changes Mar 14, 2024

View reviewed changes

Fix indentation

6bf4a96

findepi changed the title ~~Long deserializers code cleanup~~ Fix long decimal partial aggregation below join Mar 15, 2024

findepi force-pushed the findepi/long-deserializers-code-cleanup-f9dbc2 branch from 6860d9f to 112788b Compare March 15, 2024 09:07

Fix long decimal partial aggregation below join

6501e8f

Co-authored-by: Dain Sundstrom <[email protected]> Co-authored-by: Lukasz Stec <[email protected]>

Long deserializers code cleanup

7043e20

- use `Long.BYTES * n` offsets as in the serializer code (easier to correlate the two) - use same control flow pattern in the two similar classes

findepi force-pushed the findepi/long-deserializers-code-cleanup-f9dbc2 branch from 112788b to 7043e20 Compare March 15, 2024 09:39

sopel39 approved these changes Mar 15, 2024

View reviewed changes

findepi merged commit e035562 into trinodb:master Mar 15, 2024
95 checks passed

findepi deleted the findepi/long-deserializers-code-cleanup-f9dbc2 branch March 15, 2024 13:00

github-actions bot added this to the 443 milestone Mar 15, 2024

mosabua mentioned this pull request Mar 21, 2024

Add Trino 443 release notes #21178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix long decimal partial aggregation below join #21083

Fix long decimal partial aggregation below join #21083

findepi commented Mar 14, 2024 •

edited

Loading

findepi commented Mar 14, 2024

lukasz-stec left a comment

lukasz-stec Mar 14, 2024

dain Mar 14, 2024

raunaqmorarka Mar 14, 2024

dain Mar 14, 2024

dain Mar 14, 2024

findepi Mar 15, 2024

lukasz-stec Mar 14, 2024

lukasz-stec Mar 14, 2024

findepi Mar 14, 2024

dain Mar 14, 2024

sopel39 left a comment •

edited

Loading

findepi commented Mar 14, 2024

lukasz-stec commented Mar 15, 2024

findepi commented Mar 15, 2024

findepi commented Mar 15, 2024

findepi commented Mar 15, 2024

findepi commented Mar 15, 2024

sopel39 commented Mar 15, 2024 •

edited

Loading

findepi commented Mar 15, 2024

lukasz-stec commented Mar 15, 2024

findepi commented Mar 15, 2024

sopel39 left a comment

mosabua commented Mar 21, 2024

Fix long decimal partial aggregation below join #21083

Fix long decimal partial aggregation below join #21083

Conversation

findepi commented Mar 14, 2024 • edited Loading

findepi commented Mar 14, 2024

lukasz-stec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 left a comment • edited Loading

Choose a reason for hiding this comment

findepi commented Mar 14, 2024

lukasz-stec commented Mar 15, 2024

findepi commented Mar 15, 2024

findepi commented Mar 15, 2024

findepi commented Mar 15, 2024

findepi commented Mar 15, 2024

sopel39 commented Mar 15, 2024 • edited Loading

findepi commented Mar 15, 2024

lukasz-stec commented Mar 15, 2024

findepi commented Mar 15, 2024

sopel39 left a comment

Choose a reason for hiding this comment

mosabua commented Mar 21, 2024

findepi commented Mar 14, 2024 •

edited

Loading

sopel39 left a comment •

edited

Loading

sopel39 commented Mar 15, 2024 •

edited

Loading