reaggregate support for groupBy. #178

cpwright · 2025-12-10T19:43:32Z

No description provided.

github-actions · 2025-12-10T19:43:40Z

Pull Request titles must follow the Conventional Commits specification.
Breaking changes that are expected to impact users must include "BREAKING CHANGE: " at the end of the PR description.

Details:

No release type found in pull request title "reaggregate support for groupBy.". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

github-actions · 2025-12-10T19:43:41Z

No docs changes detected for `2ca4ae4`

Copilot

Pull request overview

This PR adds reaggregation support for groupBy operations in rollup tables, enabling hierarchical grouping operations to work correctly when aggregating already-aggregated data.

Key Changes:

Introduces GroupByReaggreagateOperator for handling re-aggregation of grouped data in rollup operations
Adds getAggregatedSource() method to the AggregateColumnSource interface to expose underlying aggregated sources
Adds comprehensive test coverage for both static and incremental rollup group operations

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
GroupByReaggreagateOperator.java	New operator implementing reaggregation logic for groupBy operations on already-aggregated data
AggregateColumnSource.java	Adds new interface method to expose the underlying aggregated column source
BaseAggregateColumnSource.java	Implements the new getAggregatedSource() interface method
BaseAggregateSlicedColumnSource.java	Implements the new getAggregatedSource() interface method
RangeAggregateColumnSource.java	Implements the new getAggregatedSource() interface method
AggregationProcessor.java	Integrates GroupByReaggreagateOperator into rollup aggregation processing
TestRollupTable.java	Adds test coverage for static and incremental rollup group operations
RollupTableImpl.java	Adds import (appears unused)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByReaggreagateOperator.java

...le/src/main/java/io/deephaven/engine/table/impl/sources/aggregate/AggregateColumnSource.java

engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByReaggreagateOperator.java

engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationProcessor.java

engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByReaggreagateOperator.java

engine/table/src/main/java/io/deephaven/engine/table/impl/hierarchical/RollupTableImpl.java

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByReaggregateOperator.java

...le/src/main/java/io/deephaven/engine/table/impl/sources/aggregate/AggregateColumnSource.java

Copilot · 2025-12-10T20:22:52Z

engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByReaggregateOperator.java

+            @NotNull final LongChunk<? extends RowKeys> postShiftRowKeys,
+            final long destination) {
+        // we don't need to deal with these yet
+        throw new IllegalStateException();


The error message thrown by shiftChunk should be more descriptive. It should match the message from the BucketedContext version on line 159 to maintain consistency and provide clearer information to developers.

Suggested change

throw new IllegalStateException();

throw new IllegalStateException("GroupByReaggregateOperator does not support shiftChunk; this operator does not support shifting.");

…roupByReaggregateOperator.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…ces/aggregate/AggregateColumnSource.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-11T13:29:54Z

engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationProcessor.java

+            throw new IllegalArgumentException("AggFormula does not support virtual row variables");
+        }
+    }
+


The method initializeVectorColumns is missing a JavaDoc comment explaining its purpose, parameters, and behavior. This is a non-trivial method that should be documented.

Suggested change

/**

* Initializes the {@code vectorColumnDefinitions} map if it has not already been initialized.

* 

* For each column in the provided {@code definition}, if the column is a group-by column (its name is in

* {@code groupByColumnSet}), its original {@link ColumnDefinition} is used. Otherwise, a new

* {@link ColumnDefinition} is created with a vector type corresponding to the column's data type.

* 

* After processing all columns from the definition, any additional columns from {@code extraColumns} are added.

*

* @param groupByColumnSet the set of column names used for grouping; these columns retain their original definitions

* @param definition the {@link TableDefinition} containing all columns to process

* @param extraColumns a map of additional column definitions to add to the vector column definitions

*/

Copilot · 2025-12-11T13:29:54Z

engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationProcessor.java

+            // final GroupByChunkedOperator groupByChunkedOperator = makeGroupByOperatorForFormula(inputNonKeyColumns,
+            // table);


There is a commented-out code block (lines 1378-1379) that should either be removed or replaced with a more specific TODO comment explaining why it's commented out and when it should be uncommented.

Suggested change

// final GroupByChunkedOperator groupByChunkedOperator = makeGroupByOperatorForFormula(inputNonKeyColumns,

// table);

// The following code should be restored or implemented when issue #6363 is addressed:

// final GroupByChunkedOperator groupByChunkedOperator = makeGroupByOperatorForFormula(inputNonKeyColumns, table);

Copilot · 2025-12-11T13:29:54Z

engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationProcessor.java

+        vectorColumnDefinitions.putAll(extraColumns);
+    }
+
+


There is an extra blank line that should be removed to maintain consistent code formatting.

Suggested change

Copilot · 2025-12-11T13:29:55Z

engine/table/src/main/java/io/deephaven/engine/table/impl/hierarchical/RollupTableImpl.java

     *        already filled
     * @param aggregations The aggregations
     * @param groupByColumns The group-by columns
+     * @param source


The @param source JavaDoc is missing a description. Please add a description explaining what the source parameter represents (e.g., "the source table used for reaggregation").

Suggested change

* @param source

* @param source The source table used for reaggregation.

Copilot · 2025-12-11T13:29:55Z

engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByOperator.java

+
+    /**
+     * Given that there have been modified input columns, should we propagate changes?
+     * 


The JavaDoc for the hasModifications method has a trailing space after "Given that there have been modified input columns, should we propagate changes?". This should be removed.

Suggested change

*

*

Copilot · 2025-12-11T13:29:55Z

engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationProcessor.java

+        vectorColumnDefinitions.putAll(extraColumns);
+    }
+
+


The method makeGroupByOperatorForFormula is missing a JavaDoc comment explaining its purpose, parameters, and return value. This is a non-trivial factory method that should be documented.

Suggested change

/**

* Creates a {@link GroupByChunkedOperator} for use with formula aggregations.

*

* @param inputNonKeyColumns The array of input column names (non-key columns) to be grouped.

* @param table The {@link QueryTable} on which the group by operation will be performed.

* @param exposedRowsets The name of the row set column to be exposed in the result.

* @return A {@link GroupByChunkedOperator} configured for the specified columns and table.

*/

Copilot · 2025-12-11T13:29:56Z

engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationProcessor.java

+
+    private static class WithSource extends AggregationProcessor {
+        private final @NotNull Table source;
+


The WithSource class is missing JavaDoc documentation explaining its purpose and why it's needed. Since it's extending AggregationProcessor with additional state, it should be properly documented.

Suggested change

private static class WithSource extends AggregationProcessor {

private final @NotNull Table source;

/**

* An {@link AggregationProcessor} that additionally retains a reference to the source {@link Table}.

* 

* This class is used when it is necessary to associate the aggregation processor with its originating table,

* for example, to provide context or enable operations that require access to the original data.

*

* @see AggregationProcessor

*/

private static class WithSource extends AggregationProcessor {

private final @NotNull Table source;

/**

* Constructs a WithSource instance.

*

* @param aggregations the collection of aggregations to process

* @param type the type of aggregation

* @param source the source table associated with this processor

*/

Copilot · 2025-12-11T13:29:56Z

engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationProcessor.java

            addBasicOperators((t, n) -> makeVarOrStdOperator(t, n, false, false));
        }
    }



The validateSelectColumnForFormula method is missing a JavaDoc comment explaining its purpose and when it should be used. This validation method should be documented to clarify what constraints it enforces.

Suggested change

/**

* Validates that the provided {@link SelectColumn} is suitable for use in an aggregation formula.

* 

* This method enforces the following constraints:

* <ul>

* <li>The {@code SelectColumn} must not reference column arrays.</li>

* <li>The {@code SelectColumn} must not use virtual row variables.</li>

* </ul>

* 

* This validation should be used when preparing a {@code SelectColumn} for use with {@code AggSpecFormula}

* or similar aggregation operations that do not support these features.

*

* @param selectColumn the {@code SelectColumn} to validate

* @throws IllegalArgumentException if the {@code SelectColumn} uses column arrays or virtual row variables

*/

Copilot · 2025-12-11T13:29:56Z

engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByReaggregateOperator.java

+        /* TODO: FIX THIS. */
+        return true;


There is a TODO comment indicating that the hasModifications method needs to be fixed. This should either be addressed before merging or tracked with a more specific issue reference.

Suggested change

/* TODO: FIX THIS. */

return true;

return columnsModified;

reaggregate support for groupBy.

b4cfa0e

cpwright requested a review from Copilot December 10, 2025 19:43

Copilot started reviewing on behalf of cpwright December 10, 2025 19:46 View session

Copilot AI reviewed Dec 10, 2025

View reviewed changes

copilot review

453aab1

cpwright requested a review from Copilot December 10, 2025 20:06

Copilot started reviewing on behalf of cpwright December 10, 2025 20:11 View session

Copilot AI reviewed Dec 10, 2025

View reviewed changes

cpwright and others added 3 commits December 10, 2025 15:25

Update engine/table/src/main/java/io/deephaven/engine/table/impl/by/G…

4959126

…roupByReaggregateOperator.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update engine/table/src/main/java/io/deephaven/engine/table/impl/sour…

fae01cb

…ces/aggregate/AggregateColumnSource.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

formula stuff

2ca4ae4

cpwright requested a review from Copilot December 11, 2025 13:12

Copilot started reviewing on behalf of cpwright December 11, 2025 13:17 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

	throw new IllegalStateException();
	throw new IllegalStateException("GroupByReaggregateOperator does not support shiftChunk; this operator does not support shifting.");

+/**
+ * Initializes the {@code vectorColumnDefinitions} map if it has not already been initialized.
+ * <p>
+ * For each column in the provided {@code definition}, if the column is a group-by column (its name is in
+ * {@code groupByColumnSet}), its original {@link ColumnDefinition} is used. Otherwise, a new
+ * {@link ColumnDefinition} is created with a vector type corresponding to the column's data type.
+ * <p>
+ * After processing all columns from the definition, any additional columns from {@code extraColumns} are added.
+ *
+ * @param groupByColumnSet the set of column names used for grouping; these columns retain their original definitions
+ * @param definition the {@link TableDefinition} containing all columns to process
+ * @param extraColumns a map of additional column definitions to add to the vector column definitions
+ */

		// final GroupByChunkedOperator groupByChunkedOperator = makeGroupByOperatorForFormula(inputNonKeyColumns,
		// table);

	* @param source
	* @param source The source table used for reaggregation.

+    /**
+     * Creates a {@link GroupByChunkedOperator} for use with formula aggregations.
+     *
+     * @param inputNonKeyColumns The array of input column names (non-key columns) to be grouped.
+     * @param table The {@link QueryTable} on which the group by operation will be performed.
+     * @param exposedRowsets The name of the row set column to be exposed in the result.
+     * @return A {@link GroupByChunkedOperator} configured for the specified columns and table.
+     */


		private static class WithSource extends AggregationProcessor {
		private final @NotNull Table source;

-    private static class WithSource extends AggregationProcessor {
-        private final @NotNull Table source;
+    /**
+     * An {@link AggregationProcessor} that additionally retains a reference to the source {@link Table}.
+     * <p>
+     * This class is used when it is necessary to associate the aggregation processor with its originating table,
+     * for example, to provide context or enable operations that require access to the original data.
+     *
+     * @see AggregationProcessor
+     */
+    private static class WithSource extends AggregationProcessor {
+        private final @NotNull Table source;
+        /**
+         * Constructs a WithSource instance.
+         *
+         * @param aggregations the collection of aggregations to process
+         * @param type the type of aggregation
+         * @param source the source table associated with this processor
+         */

+    /**
+     * Validates that the provided {@link SelectColumn} is suitable for use in an aggregation formula.
+     * <p>
+     * This method enforces the following constraints:
+     * <ul>
+     *     <li>The {@code SelectColumn} must not reference column arrays.</li>
+     *     <li>The {@code SelectColumn} must not use virtual row variables.</li>
+     * </ul>
+     * <p>
+     * This validation should be used when preparing a {@code SelectColumn} for use with {@code AggSpecFormula}
+     * or similar aggregation operations that do not support these features.
+     *
+     * @param selectColumn the {@code SelectColumn} to validate
+     * @throws IllegalArgumentException if the {@code SelectColumn} uses column arrays or virtual row variables
+     */

reaggregate support for groupBy. #178

Are you sure you want to change the base?

reaggregate support for groupBy. #178

Uh oh!

Conversation

cpwright commented Dec 10, 2025

Uh oh!

github-actions bot commented Dec 10, 2025

Uh oh!

github-actions bot commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No docs changes detected for 2ca4ae4

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Dec 10, 2025 •

edited

Loading

No docs changes detected for `2ca4ae4`