-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use builders for GroupBy
initialization and updates
#2886
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As-written, this only speeds up the instantiation pass. For partition-by, since we already needed to keep track of adds, removes, and modifies each cycle, changing update processing was a no-brainer. Do you think it would be worthwhile to start using random builders in updating processing to track adds and removes in group-by? I suspect the trade-off to be worth it.
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
A thing I've just thought of for group-by and partition-by. If we're using initial groups and !preserveEmpty, we might be orphaning empty builders in the addedBuilders sources indefinitely, since we only do any work with the ones that make it into the initial row set. I'm not sure it's a big deal, but it's not optimal. I think we could pass the max slot used into |
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is basically ready to merge, if nightlies look good. Minor nitpicks only.
engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationContext.java
Outdated
Show resolved
Hide resolved
.../table/src/main/java/io/deephaven/engine/table/impl/by/ChunkedOperatorAggregationHelper.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByChunkedOperator.java
Outdated
Show resolved
Hide resolved
engine/rowset/src/main/java/io/deephaven/engine/rowset/impl/BitmapRandomBuilder.java
Outdated
Show resolved
Hide resolved
GroupBy
initializationGroupBy
initialization and updates
Similar to #2852, change
GroupByChunkedOperator
to useRowSetSequentialBuilder
during the initial creation of thegroupBy
table.Closes #2873