Add support for Spilling of Distinct Aggregations #7791

aditi-pandit · 2023-11-29T16:45:23Z

HashAggregation supports spilling in general. However, it didn't support spilling for a DISTINCT aggregation. e.g. Like SQL query

SELECT c1, count(DISTINCT c0) FROM tmp GROUP BY c1

Distinct aggregations work by capturing the column parameter values in a SetAccumulator to track only the DISTINCT values. The outer aggregation is computed over the contents of these SetAccumulators.

To support spilling in this context multiple changes are needed:

Enhance the RowContainer/GroupingSet code for spill extraction and re-building aggregation contents from the spill to perform these functions for the DistinctAggregations as well.
Add capabilities to extract spill contents and rebuild accumulators from previous spill in the SetAccumulators.
SetAccumulators are of 3 types : i) For fixed-width types ii) String types iii) Complex types. Each of these are enhanced to extract spill and add from spill contents. The spill extraction involves serializing to a blob the set accumulator contents in the order of their indices captured from the input streams. String SetAccumulators serialize (length + contents) for each entry. ComplexType SetAccumulators serialize (length + contents) as well.
Serializing ComplexTypes involves using an AddressableNonNullValueList structure.
-- This class needed new methods to copy what appears externally a contiguous set of bytes into a ByteStream to extract a spill from it.
-- A new method to append a stream of previously serialized contents from the spill was also added.
-- Since we need to serialize the length of each ComplexType entry in the SetAccumulator the append method was enhanced as well to return the number of serialized bytes for it as well.
Exhaustive tests for the SetAccumulator spill extraction and reconstruction are added.

netlify · 2023-11-29T16:45:29Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`37751f9`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/671bdf123c99940009b58156

mbasmanova · 2024-01-08T23:24:49Z

@aditi-pandit Aditi, I see CI failures. Is this PR ready for review? How is it different from #8287 ?

aditi-pandit · 2024-01-09T03:49:11Z

@aditi-pandit Aditi, I see CI failures. Is this PR ready for review? How is it different from #8287 ?

@mbasmanova : This PR is ready for review. Looking at the test failure.

The code in this PR seemed simpler in both the DistinctAggregations and serialization logic for maintaining lengths. Also has more tests for ComplexType serialization. My apologies to @supermem613, this had been in the works for some time and I should've linked it in the issue.

mbasmanova · 2024-01-09T12:19:21Z

@aditi-pandit

The code in this PR seemed simpler in both the DistinctAggregations and serialization logic for maintaining lengths.

Would you clarify a bit more how this PR works and how it is different / better than the other PR? PR description doesn't have any details and it will take significant amount of time and effort to reverse engineer the design from the code itself.

aditi-pandit · 2024-01-10T19:05:13Z

@aditi-pandit

The code in this PR seemed simpler in both the DistinctAggregations and serialization logic for maintaining lengths.

Would you clarify a bit more how this PR works and how it is different / better than the other PR? PR description doesn't have any details and it will take significant amount of time and effort to reverse engineer the design from the code itself.

@mbasmanova : Have added a detailed description of the changes I made.

aditi-pandit · 2024-01-10T19:07:01Z

@mbasmanova : Have addressed your review comments. PTAL.

mbasmanova

@aditi-pandit Aditi, thank you for adding a description. This is very helpful. It sounds like it would be helpful to extract changes to SetAccumulator and AddressableNonNullValueList into 2 separate PRs.

mbasmanova · 2024-01-10T20:36:22Z

velox/exec/SetAccumulator.h

+  void addNullIndex(const char* buffer) {
+    VELOX_CHECK(!nullIndex.has_value());
+    vector_size_t serializedNullIndex;
+    memcpy(&serializedNullIndex, buffer, kVectorSizeT);


There are InputByteStream and OutputByteStream classes in velox/common/base/IOUtils.h which can be used here to make the code easier to read.

I was able to use InputByteStream for deserialization functions. But since we random access during serialization(write), I didn't use OutputByteStream.

velox/exec/SetAccumulator.h

mbasmanova · 2024-01-10T20:40:08Z

velox/exec/SetAccumulator.h

+      HashStringAllocator* /*allocator*/) {
+    VELOX_CHECK(!vector.isNullAt(i));
+
+    // The serialized value is the nullOffset (kNoNullIndex if no null is


It would be nice to document the serialization format in one place and avoid repeating it in comments.

mbasmanova · 2024-01-10T20:40:45Z

velox/exec/SetAccumulator.h

+    // The serialized value is the nullOffset (kNoNullIndex if no null is
+    // present) followed by the unique values ordered by index.
+    auto serialized = vector.valueAt(i);
+    auto size = serialized.size();


perhaps, include number of entries in the serialized data for extra safety

velox/exec/SetAccumulator.h

Yuhta

Oops, stamp the wrong PR

…f bytes (#8653) Summary: This is the first in a set of PRs to add support for spilling distinct aggregations (see full version in #7791). Spilling distinct aggregations needs support to spill SetAccumulators in which input values are cumulated. ComplexTypeSetAccumulators use AddressableNonNullValueList to serialize complex types. This PR adds new APIs to AddressableNonNullValueList so that it can copy/append a stream of bytes corresponding to a ComplexType value (array, map, struct). Pull Request resolved: #8653 Reviewed By: Yuhta Differential Revision: D53497000 Pulled By: mbasmanova fbshipit-source-id: 66d44d02a2c3bd5775725c8b8559feaed17c0813

…f bytes (facebookincubator#8653) Summary: This is the first in a set of PRs to add support for spilling distinct aggregations (see full version in facebookincubator#7791). Spilling distinct aggregations needs support to spill SetAccumulators in which input values are cumulated. ComplexTypeSetAccumulators use AddressableNonNullValueList to serialize complex types. This PR adds new APIs to AddressableNonNullValueList so that it can copy/append a stream of bytes corresponding to a ComplexType value (array, map, struct). Pull Request resolved: facebookincubator#8653 Reviewed By: Yuhta Differential Revision: D53497000 Pulled By: mbasmanova fbshipit-source-id: 66d44d02a2c3bd5775725c8b8559feaed17c0813

stale · 2024-05-28T11:30:53Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

aditi-pandit · 2024-10-24T19:01:26Z

@xiaoxmeng, @Yuhta : This is the PR for Spilling of Distinct Aggregations. Lets continue the review here.

format-fix

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 29, 2023

aditi-pandit marked this pull request as draft November 29, 2023 16:45

aditi-pandit force-pushed the spill_distinct branch 5 times, most recently from 8941cdc to ef8c5f6 Compare November 30, 2023 07:26

aditi-pandit force-pushed the spill_distinct branch from ef8c5f6 to c3752b8 Compare December 22, 2023 03:00

This was referenced Jan 8, 2024

Add spilling support for aggregations with distinct. #7454

Open

Support Spill for Distinct Aggregation #8287

Closed

aditi-pandit force-pushed the spill_distinct branch 7 times, most recently from 7822210 to 641ff8c Compare January 8, 2024 18:02

aditi-pandit requested review from mbasmanova and xiaoxmeng January 8, 2024 18:13

aditi-pandit marked this pull request as ready for review January 8, 2024 18:13

aditi-pandit force-pushed the spill_distinct branch from 641ff8c to 1baf80b Compare January 9, 2024 03:27

aditi-pandit force-pushed the spill_distinct branch 5 times, most recently from a50bf39 to 75a6721 Compare January 9, 2024 06:20

aditi-pandit force-pushed the spill_distinct branch 2 times, most recently from 60a3b98 to 3b99863 Compare January 10, 2024 18:31

mbasmanova reviewed Jan 10, 2024

View reviewed changes

mbasmanova mentioned this pull request Jan 11, 2024

Add aggregations over distinct inputs to AggregationFuzzer #8328

Closed

aditi-pandit force-pushed the spill_distinct branch 4 times, most recently from 6acb842 to 015ac0a Compare February 2, 2024 06:54

This was referenced Feb 2, 2024

Add new APIs to AddressableNonNullValueList to copy/append a stream of bytes #8653

Closed

[Do not review. Prefer https://github.com/facebookincubator/velox/pull/8660] Add logic to serialize/deserialize SetAccumulators #8660

Closed

Yuhta approved these changes Feb 7, 2024

View reviewed changes

Yuhta requested changes Feb 7, 2024

View reviewed changes

aditi-pandit force-pushed the spill_distinct branch from 015ac0a to c1891b9 Compare February 28, 2024 07:13

stale bot added the stale label May 28, 2024

stale bot closed this Jun 12, 2024

aditi-pandit reopened this Oct 23, 2024

aditi-pandit requested review from assignUser and majetideepak as code owners October 23, 2024 08:36

aditi-pandit force-pushed the spill_distinct branch from c1891b9 to d45f971 Compare October 24, 2024 03:09

stale bot removed the stale label Oct 24, 2024

Add support for spilling distinct aggregations

37751f9

format-fix

aditi-pandit force-pushed the spill_distinct branch from d45f971 to 37751f9 Compare October 25, 2024 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Spilling of Distinct Aggregations #7791

Add support for Spilling of Distinct Aggregations #7791

aditi-pandit commented Nov 29, 2023 •

edited

Loading

netlify bot commented Nov 29, 2023 •

edited

Loading

mbasmanova commented Jan 8, 2024

aditi-pandit commented Jan 9, 2024

mbasmanova commented Jan 9, 2024

aditi-pandit commented Jan 10, 2024

aditi-pandit commented Jan 10, 2024

mbasmanova left a comment

mbasmanova Jan 10, 2024

aditi-pandit Feb 2, 2024

mbasmanova Jan 10, 2024

mbasmanova Jan 10, 2024

Yuhta left a comment

stale bot commented May 28, 2024

aditi-pandit commented Oct 24, 2024

Add support for Spilling of Distinct Aggregations #7791

Are you sure you want to change the base?

Add support for Spilling of Distinct Aggregations #7791

Conversation

aditi-pandit commented Nov 29, 2023 • edited Loading

netlify bot commented Nov 29, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

mbasmanova commented Jan 8, 2024

aditi-pandit commented Jan 9, 2024

mbasmanova commented Jan 9, 2024

aditi-pandit commented Jan 10, 2024

aditi-pandit commented Jan 10, 2024

mbasmanova left a comment

Choose a reason for hiding this comment

mbasmanova Jan 10, 2024

Choose a reason for hiding this comment

aditi-pandit Feb 2, 2024

Choose a reason for hiding this comment

mbasmanova Jan 10, 2024

Choose a reason for hiding this comment

mbasmanova Jan 10, 2024

Choose a reason for hiding this comment

Yuhta left a comment

Choose a reason for hiding this comment

stale bot commented May 28, 2024

aditi-pandit commented Oct 24, 2024

aditi-pandit commented Nov 29, 2023 •

edited

Loading

netlify bot commented Nov 29, 2023 •

edited

Loading