chore(sql): add GroupByArraySink by mcadariu · Pull Request #6237 · questdb/questdb

mcadariu · 2025-10-06T19:07:40Z

Relates to #4120.

In #4097, a specialized allocator for aggregate functions was introduced and a couple of them migrated to use it.

This PR follows-up by adding GroupByArraySink which we will then use in a subsequent PR to port last(array).

coderabbitai · 2025-10-06T19:08:07Z

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

core/src/main/java/io/questdb/griffin/engine/groupby/GroupByArraySink.java

… of 8, as well as ask the caller to provide the type.

core/src/main/java/io/questdb/griffin/engine/groupby/GroupByArraySink.java

core/src/test/java/io/questdb/test/griffin/engine/groupby/GroupByArraySinkTest.java

…the ArrayTypeDriver

core/src/main/java/io/questdb/cairo/arr/ArrayTypeDriver.java

core/src/main/java/io/questdb/griffin/engine/groupby/GroupByArraySink.java

core/src/test/java/io/questdb/test/griffin/engine/groupby/GroupByArraySinkTest.java

…bleValue()) to the appendCompactPlainArray

…ew compact layout and the existing one

… This is because the previous approach using getFlatViewLength can only be trusted with a vanilla array.

core/src/test/java/io/questdb/test/griffin/engine/groupby/GroupByArraySinkTest.java

puzpuzpuz · 2025-10-15T15:19:18Z

Linter step is unhappy with javadoc formatting. Could you add the missing padding?

--- a/core/src/main/java/io/questdb/cairo/arr/ArrayTypeDriver.java
+++ b/core/src/main/java/io/questdb/cairo/arr/ArrayTypeDriver.java
@@ -179,9 +179,9 @@ public class ArrayTypeDriver implements ColumnTypeDriver {
      * and a 4-byte type field. This compact format omits the type (known from context) and
      * uses a 4-byte dataSize instead of 8-byte totalSize.
      *
-     * @param addr the memory address to write to
-     * @param value the array to append
-     * @param nDims the number of dimensions
+     * @param addr     the memory address to write to
+     * @param value    the array to append
+     * @param nDims    the number of dimensions
      * @param elemSize the size of each element in bytes
      */
     public static void appendCompactPlainValue(long addr, ArrayView value, int nDims, int elemSize) {
@@ -342,8 +342,8 @@ public class ArrayTypeDriver implements ColumnTypeDriver {
      * This is the counterpart to {@link #appendCompactPlainValue} and differs from {@link #getPlainValue}
      * which expects an 8-byte size field and a type field in the layout.
      *
-     * @param addr the memory address to read from
-     * @param type the encoded array type (provided from context)
+     * @param addr  the memory address to read from
+     * @param type  the encoded array type (provided from context)
      * @param nDims the number of dimensions
      * @param value the borrowed array to populate
      * @return the populated array

puzpuzpuz · 2025-10-15T15:19:45Z

The PR is almost there. Great job!

puzpuzpuz

Thanks for the solid contribution!

mcadariu · 2025-10-17T13:40:45Z

Thanks for the clear and quick feedback on this PR!

mcadariu · 2025-10-20T11:17:00Z

@puzpuzpuz thanks for the approval! Is there anything else I can do for this PR? If it gets merged, I can proceed with doing last(array).

puzpuzpuz · 2025-10-20T11:31:44Z

@puzpuzpuz thanks for the approval! Is there anything else I can do for this PR? If it gets merged, I can proceed with doing last(array).

Nope, nothing to do here. I don't have the merge permissions and the rest of the team is pretty busy, so it'll take some time. You could create a separate PR that is dependent on this one and, once this one is merged, simply do a rebase.

mcadariu · 2025-10-20T12:36:34Z

Got it, thanks!

Add GroupByArraySink and tests

0484a28

mcadariu force-pushed the add_group_by_array_sink branch from de72815 to 0484a28 Compare October 7, 2025 11:01

mcadariu mentioned this pull request Oct 7, 2025

Port remaining parallelizable aggregate functions to off-heap data structures #4120

Open

6 tasks

puzpuzpuz reviewed Oct 7, 2025

View reviewed changes

core/src/main/java/io/questdb/griffin/engine/groupby/GroupByArraySink.java Outdated Show resolved Hide resolved

core/src/main/java/io/questdb/griffin/engine/groupby/GroupByArraySink.java Outdated Show resolved Hide resolved

mcadariu added 2 commits October 8, 2025 13:22

To be more memory-efficient we encode total size with 4 bytes instead…

1d7db8d

… of 8, as well as ask the caller to provide the type.

cosmetic fixes

f108ddd

puzpuzpuz added the Enhancement Enhance existing functionality label Oct 10, 2025

puzpuzpuz reviewed Oct 10, 2025

View reviewed changes

core/src/main/java/io/questdb/griffin/engine/groupby/GroupByArraySink.java Outdated Show resolved Hide resolved

puzpuzpuz reviewed Oct 10, 2025

View reviewed changes

core/src/main/java/io/questdb/griffin/engine/groupby/GroupByArraySink.java Show resolved Hide resolved

puzpuzpuz reviewed Oct 10, 2025

View reviewed changes

core/src/test/java/io/questdb/test/griffin/engine/groupby/GroupByArraySinkTest.java Outdated Show resolved Hide resolved

mcadariu added 6 commits October 11, 2025 11:06

Move memory writing and reading out of the GroupByArraySink and into …

55527b6

…the ArrayTypeDriver

Cache ndims and headerSize to avoid unnecessary computations

cd915c2

Remove unused argument

6825cd5

Cache the element size as well

998a670

Update tests to use BorrowedArray instead of ArrayTypeDriver method

ff303ec

simplify computations

c24819b

puzpuzpuz reviewed Oct 13, 2025

View reviewed changes

core/src/main/java/io/questdb/cairo/arr/ArrayTypeDriver.java Outdated Show resolved Hide resolved

core/src/main/java/io/questdb/cairo/arr/ArrayTypeDriver.java Outdated Show resolved Hide resolved

puzpuzpuz reviewed Oct 13, 2025

View reviewed changes

core/src/main/java/io/questdb/griffin/engine/groupby/GroupByArraySink.java Outdated Show resolved Hide resolved

puzpuzpuz reviewed Oct 13, 2025

View reviewed changes

core/src/test/java/io/questdb/test/griffin/engine/groupby/GroupByArraySinkTest.java Show resolved Hide resolved

mcadariu added 8 commits October 14, 2025 09:51

Bring in fast-path implementation usage (FlatArrayView#appendPlainDou…

c2a7e6a

…bleValue()) to the appendCompactPlainArray

Add Javadoc comment describing what are the differences between the n…

efd2745

…ew compact layout and the existing one

Introduce the getCompactPlainValueSize method in the ArrayTypeDriver.…

8696bb5

… This is because the previous approach using getFlatViewLength can only be trusted with a vanilla array.

Improve consistency between compact and non-compact versions of methods

6ae801b

Add smoke test which covers DerivedArray

bee29dd

Use getCardinality instead of getFlatViewLength for non-vanilla path

22132bf

Tweaking the javadoc

56be78f

Consistency: nDims everywhere

1fd6d87

puzpuzpuz reviewed Oct 15, 2025

View reviewed changes

core/src/test/java/io/questdb/test/griffin/engine/groupby/GroupByArraySinkTest.java Show resolved Hide resolved

mcadariu added 3 commits October 16, 2025 13:01

Verify sink contents in testPutNonVanillaArray

be53fb3

Fix layout of the javadocs

74d9379

assert the contents of derivedArray against result

18c10ea

puzpuzpuz approved these changes Oct 17, 2025

View reviewed changes

Merge branch 'master' into add_group_by_array_sink

a554736

mcadariu mentioned this pull request Oct 21, 2025

feat(sql): add last(array) function #6291

Merged

bluestreak01 changed the title ~~Add GroupByArraySink~~ chore(sql): add GroupByArraySink Oct 23, 2025

bluestreak01 merged commit 4fd2185 into questdb:master Oct 23, 2025
25 checks passed

mcadariu mentioned this pull request Oct 28, 2025

chore(sql): port first(array) to use array sink #6322

Merged

Conversation

mcadariu commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

puzpuzpuz commented Oct 15, 2025

Uh oh!

puzpuzpuz commented Oct 15, 2025

Uh oh!

puzpuzpuz left a comment

Choose a reason for hiding this comment

Uh oh!

mcadariu commented Oct 17, 2025

Uh oh!

mcadariu commented Oct 20, 2025

Uh oh!

puzpuzpuz commented Oct 20, 2025

Uh oh!

mcadariu commented Oct 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mcadariu commented Oct 6, 2025 •

edited

Loading

coderabbitai bot commented Oct 6, 2025 •

edited

Loading