feat: add multi-column support to `UpdateBy` `RollingFormula()` operator #6143

lbooker42 · 2024-09-26T21:02:52Z

No description provided.

malhotrashivam

Code review Part 1: All replicated files and files with smaller changes

Base/src/main/java/io/deephaven/base/ringbuffer/RingBuffer.java

...gine/table/impl/updateby/rollingformula/ringbuffervectorwrapper/RingBufferVectorWrapper.java

malhotrashivam · 2024-10-03T17:59:50Z

...gine/table/impl/updateby/rollingformula/ringbuffervectorwrapper/RingBufferVectorWrapper.java

+            // noinspection unchecked
+            result = new ObjectRingBufferVectorWrapper<T>((ObjectRingBuffer<T>) buffer, (Class<T>) componentType);


Please check to get full test coverage.

...a/io/deephaven/engine/table/impl/updateby/rollingminmax/ComparableRollingMinMaxOperator.java

...e/table/impl/updateby/rollingformulamulticolumn/windowconsumer/RingBufferWindowConsumer.java

...ble/impl/updateby/rollingformulamulticolumn/windowconsumer/CharRingBufferWindowConsumer.java

...e/impl/updateby/rollingformulamulticolumn/windowconsumer/ObjectRingBufferWindowConsumer.java

table-api/src/main/java/io/deephaven/api/updateby/UpdateByOperation.java

engine/table/src/test/java/io/deephaven/engine/table/impl/updateby/TestRollingFormula.java

engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java

table-api/src/main/java/io/deephaven/api/updateby/spec/RollingFormulaSpec.java

.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java

Base/src/main/java/io/deephaven/base/ringbuffer/RingBuffer.java

...va/io/deephaven/engine/table/impl/updateby/rollingformula/BooleanRollingFormulaOperator.java

table-api/src/main/java/io/deephaven/api/updateby/spec/RollingFormulaSpec.java

.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java

lbooker42 · 2024-10-17T17:46:26Z

Python support for this is incomplete, need to update function signatures and documentation, plus include examples.
Have added this support in the latest commit.

Base/src/main/java/io/deephaven/base/ringbuffer/ObjectRingBuffer.java

engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperator.java

engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java

rcaudy

Looking pretty good. We can take a more thorough/complete pass next week if needed, but I don't want to duplicate effort too much. Do want to look at things after refactoring to rely more on SelectColumn and Selectable.

.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java

…y value.

…entity values.

rcaudy

Just need to look at the operator code (base and multi). Deferring Python.

Base/src/main/java/io/deephaven/base/ringbuffer/CharRingBuffer.java

Base/src/main/java/io/deephaven/base/ringbuffer/ObjectRingBuffer.java

...e/table/impl/updateby/rollingformulamulticolumn/windowconsumer/RingBufferWindowConsumer.java

table-api/src/main/java/io/deephaven/api/updateby/UpdateByOperation.java

table-api/src/main/java/io/deephaven/api/updateby/spec/RollingFormulaSpec.java

engine/table/src/test/java/io/deephaven/engine/table/impl/updateby/TestRollingFormula.java

rcaudy · 2024-10-23T22:16:01Z

engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java

+                vectorColumnNameMap = new HashMap<>();
+                columnDefinitionMap.forEach((key, value) -> {
+                    final ColumnDefinition<?> columnDef = ColumnDefinition.fromGenericType(
+                            key, VectorFactory.forElementType(value.getDataType()).vectorType());


Add component type (that is, value.getDataType()). I wonder if that means we need more testing for Object inputs.

This is done, thinking about add'l testing

engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java

Base/src/main/java/io/deephaven/base/ringbuffer/ObjectRingBuffer.java

table-api/src/main/java/io/deephaven/api/updateby/spec/RollingFormulaSpec.java

rcaudy

Pretty close to mergeable. I kind of wonder if we should be deleting the single-input implementation and always using the new one.

table-api/src/main/java/io/deephaven/api/updateby/spec/RollingFormulaSpec.java

.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java

rcaudy · 2024-10-30T21:06:33Z

.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java

+
+    @Override
+    public void applyOutputShift(@NotNull final RowSet subRowSetToShift, final long delta) {
+        ((SparseArrayColumnSource<?>) outputSource).shift(subRowSetToShift, delta);


This may break if you have a reinterpret. Be sure to test shifts + reinterpret.
I think you're already OK... but a test might help prove it.

.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java

engine/table/src/main/java/io/deephaven/engine/table/impl/select/SelectColumn.java

engine/table/src/main/java/io/deephaven/engine/table/impl/sources/ObjectSingleValueSource.java

deephaven-internal · 2024-11-01T17:13:30Z

Labels indicate documentation is required. Issues for documentation have been opened:

Community: deephaven/deephaven-docs-community#352

lbooker42 added 2 commits September 16, 2024 15:31

Refactor to allow dynamic affectingColumns in the base class.

265e334

Working, tests passing but need more.

5c5294f

lbooker42 added DocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Sep 26, 2024

lbooker42 added this to the 0.37.0 milestone Sep 26, 2024

lbooker42 self-assigned this Sep 26, 2024

rcaudy requested review from rcaudy and malhotrashivam September 26, 2024 22:00

Added support for multiple formulas per call, spotless.

c8c87bd

malhotrashivam reviewed Oct 3, 2024

View reviewed changes

malhotrashivam reviewed Oct 4, 2024

View reviewed changes

Addressed PR comments

b64b342

lbooker42 changed the title ~~Add multi-column support to UpdateBy RollingFormula() operator~~ feat: add multi-column support to UpdateBy RollingFormula() operator Oct 9, 2024

Corrected JavaDoc failure in RingBufferWindowConsumer

66dff62

malhotrashivam reviewed Oct 9, 2024

View reviewed changes

Addressed PR comments and added python implementation.

eaa2816

lbooker42 requested review from chipkent and jmao-denver as code owners October 17, 2024 21:02

lbooker42 added 2 commits October 17, 2024 18:02

Updated python tests.

37298c4

Minor updates.

efc533c

rcaudy reviewed Oct 18, 2024

View reviewed changes

.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java Outdated Show resolved Hide resolved

lbooker42 added 3 commits October 21, 2024 11:44

Refactor to use SelectColumn instead of FormulaColumn

c969c11

Aggregating ring buffer correction to reset cleared buffer to identit…

b3a02fb

…y value.

Change AggregatingRingBuffer code to be smarter about resetting to id…

36756da

…entity values.

lbooker42 force-pushed the lab-rolling-multi-formula branch from ef9aab7 to 36756da Compare October 23, 2024 21:40

deephaven deleted a comment from github-actions bot Oct 23, 2024

rcaudy reviewed Oct 23, 2024

View reviewed changes

PR comment and ring buffer cleanup and tests.

4b0eb3f

rcaudy reviewed Oct 25, 2024

View reviewed changes

Addressed PR comments.

92b9a2c

rcaudy reviewed Oct 30, 2024

View reviewed changes

lbooker42 added 2 commits October 30, 2024 18:03

WIP

d98a36b

Nomenclature upgrade.

e324f21

rcaudy reviewed Oct 31, 2024

View reviewed changes

engine/table/src/main/java/io/deephaven/engine/table/impl/select/SelectColumn.java Outdated Show resolved Hide resolved

engine/table/src/main/java/io/deephaven/engine/table/impl/sources/ObjectSingleValueSource.java Outdated Show resolved Hide resolved

Re-ran replicateSourcesAndChunks and corrected typo.

b990fd8

rcaudy previously approved these changes Oct 31, 2024

View reviewed changes

Re-ran replicateSourcesAndChunks.

eef3cd5

lbooker42 dismissed rcaudy’s stale review via eef3cd5 October 31, 2024 20:52

rcaudy approved these changes Oct 31, 2024

View reviewed changes

lbooker42 merged commit c9bb557 into deephaven:main Nov 1, 2024
17 checks passed

github-actions bot locked and limited conversation to collaborators Nov 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add multi-column support to `UpdateBy` `RollingFormula()` operator #6143

feat: add multi-column support to `UpdateBy` `RollingFormula()` operator #6143

lbooker42 commented Sep 26, 2024

malhotrashivam left a comment

malhotrashivam Oct 3, 2024

lbooker42 commented Oct 17, 2024 •

edited

Loading

rcaudy left a comment

rcaudy left a comment

rcaudy Oct 23, 2024

lbooker42 Oct 25, 2024

rcaudy left a comment

rcaudy Oct 30, 2024

deephaven-internal commented Nov 1, 2024

		// noinspection unchecked
		result = new ObjectRingBufferVectorWrapper<T>((ObjectRingBuffer<T>) buffer, (Class<T>) componentType);

feat: add multi-column support to UpdateBy RollingFormula() operator #6143

feat: add multi-column support to UpdateBy RollingFormula() operator #6143

Conversation

lbooker42 commented Sep 26, 2024

malhotrashivam left a comment

Choose a reason for hiding this comment

malhotrashivam Oct 3, 2024

Choose a reason for hiding this comment

lbooker42 commented Oct 17, 2024 • edited Loading

rcaudy left a comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

rcaudy Oct 23, 2024

Choose a reason for hiding this comment

lbooker42 Oct 25, 2024

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

rcaudy Oct 30, 2024

Choose a reason for hiding this comment

deephaven-internal commented Nov 1, 2024

feat: add multi-column support to `UpdateBy` `RollingFormula()` operator #6143

feat: add multi-column support to `UpdateBy` `RollingFormula()` operator #6143

lbooker42 commented Oct 17, 2024 •

edited

Loading