fix!: DH-18471 update_by min/max functions to return original-typed output columns (#6629)

lbooker42 · web-flow · commit 1feacf46bd2d · 2025-02-07T11:10:15.000-08:00
Add'l work performed:

- Added `char` support to `CumMin`/`CumMax` and expanded tests
- Added PartionedTable proxy tests
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java
@@ -856,6 +856,8 @@ private UpdateByOperator makeCumMinMaxOperator(MatchPair pair, TableDefinition t
 
             if (csType == byte.class || csType == Byte.class) {
                 return new ByteCumMinMaxOperator(pair, isMax, NULL_BYTE);
+            } else if (csType == char.class || csType == Character.class) {
+                return new CharCumMinMaxOperator(pair, isMax);
             } else if (csType == short.class || csType == Short.class) {
                 return new ShortCumMinMaxOperator(pair, isMax);
             } else if (csType == int.class || csType == Integer.class) {
@@ -1033,7 +1035,7 @@ private UpdateByOperator makeRollingGroupOperator(@NotNull final MatchPair[] pai
 
             return new RollingGroupOperator(pairs, affectingColumns,
                     rg.revWindowScale().timestampCol(),
-                    prevWindowScaleUnits, fwdWindowScaleUnits, tableDef);
+                    prevWindowScaleUnits, fwdWindowScaleUnits);
         }
 
         private UpdateByOperator makeRollingAvgOperator(@NotNull final MatchPair pair,
@@ -1132,7 +1134,7 @@ private UpdateByOperator makeRollingMinMaxOperator(@NotNull MatchPair pair,
             } else if (csType == long.class || csType == Long.class || isTimeType(csType)) {
                 return new LongRollingMinMaxOperator(pair, affectingColumns,
                         rmm.revWindowScale().timestampCol(),
-                        prevWindowScaleUnits, fwdWindowScaleUnits, rmm.isMax());
+                        prevWindowScaleUnits, fwdWindowScaleUnits, rmm.isMax(), csType);
             } else if (csType == float.class || csType == Float.class) {
                 return new FloatRollingMinMaxOperator(pair, affectingColumns,
                         rmm.revWindowScale().timestampCol(),
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/ByteCumMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/ByteCumMinMaxOperator.java
@@ -2,7 +2,7 @@
 // Copyright (c) 2016-2025 Deephaven Data Labs and Patent Pending
 //
 // ****** AUTO-GENERATED CLASS - DO NOT EDIT MANUALLY
-// ****** Edit ShortCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
+// ****** Edit CharCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
 //
 // @formatter:off
 package io.deephaven.engine.table.impl.updateby.minmax;
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/CharCumMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/CharCumMinMaxOperator.java
@@ -0,0 +1,82 @@
+//
+// Copyright (c) 2016-2025 Deephaven Data Labs and Patent Pending
+//
+package io.deephaven.engine.table.impl.updateby.minmax;
+
+import io.deephaven.base.verify.Assert;
+import io.deephaven.chunk.Chunk;
+import io.deephaven.chunk.CharChunk;
+import io.deephaven.chunk.attributes.Values;
+import io.deephaven.engine.table.impl.MatchPair;
+import io.deephaven.engine.table.impl.updateby.UpdateByOperator;
+import io.deephaven.engine.table.impl.updateby.internal.BaseCharUpdateByOperator;
+import org.jetbrains.annotations.NotNull;
+
+import static io.deephaven.util.QueryConstants.*;
+
+public class CharCumMinMaxOperator extends BaseCharUpdateByOperator {
+    private final boolean isMax;
+
+    // region extra-fields
+    // endregion extra-fields
+
+    protected class Context extends BaseCharUpdateByOperator.Context {
+        public CharChunk<? extends Values> charValueChunk;
+
+        protected Context(final int chunkSize) {
+            super(chunkSize);
+        }
+
+        @Override
+        public void setValueChunks(@NotNull final Chunk<? extends Values>[] valueChunks) {
+            charValueChunk = valueChunks[0].asCharChunk();
+        }
+
+        @Override
+        public void push(int pos, int count) {
+            Assert.eq(count, "push count", 1);
+
+            final char val = charValueChunk.get(pos);
+
+            if (curVal == NULL_CHAR) {
+                curVal = val;
+            } else if (val != NULL_CHAR) {
+                if ((isMax && val > curVal) ||
+                        (!isMax && val < curVal)) {
+                    curVal = val;
+                }
+            }
+        }
+    }
+
+    public CharCumMinMaxOperator(
+            @NotNull final MatchPair pair,
+            final boolean isMax
+    // region extra-constructor-args
+    // endregion extra-constructor-args
+    ) {
+        super(pair, new String[] {pair.rightColumn});
+        this.isMax = isMax;
+        // region constructor
+        // endregion constructor
+    }
+
+    @Override
+    public UpdateByOperator copy() {
+        return new CharCumMinMaxOperator(
+                pair,
+                isMax
+        // region extra-copy-args
+        // endregion extra-copy-args
+        );
+    }
+
+    @NotNull
+    @Override
+    public UpdateByOperator.Context makeUpdateContext(final int affectedChunkSize, final int influencerChunkSize) {
+        return new Context(affectedChunkSize);
+    }
+
+    // region extra-methods
+    // endregion extra-methods
+}
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/DoubleCumMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/DoubleCumMinMaxOperator.java
@@ -2,7 +2,7 @@
 // Copyright (c) 2016-2025 Deephaven Data Labs and Patent Pending
 //
 // ****** AUTO-GENERATED CLASS - DO NOT EDIT MANUALLY
-// ****** Edit ShortCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
+// ****** Edit CharCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
 //
 // @formatter:off
 package io.deephaven.engine.table.impl.updateby.minmax;
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/FloatCumMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/FloatCumMinMaxOperator.java
@@ -2,7 +2,7 @@
 // Copyright (c) 2016-2025 Deephaven Data Labs and Patent Pending
 //
 // ****** AUTO-GENERATED CLASS - DO NOT EDIT MANUALLY
-// ****** Edit ShortCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
+// ****** Edit CharCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
 //
 // @formatter:off
 package io.deephaven.engine.table.impl.updateby.minmax;
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/IntCumMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/IntCumMinMaxOperator.java
@@ -2,7 +2,7 @@
 // Copyright (c) 2016-2025 Deephaven Data Labs and Patent Pending
 //
 // ****** AUTO-GENERATED CLASS - DO NOT EDIT MANUALLY
-// ****** Edit ShortCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
+// ****** Edit CharCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
 //
 // @formatter:off
 package io.deephaven.engine.table.impl.updateby.minmax;
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/LongCumMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/LongCumMinMaxOperator.java
@@ -2,7 +2,7 @@
 // Copyright (c) 2016-2025 Deephaven Data Labs and Patent Pending
 //
 // ****** AUTO-GENERATED CLASS - DO NOT EDIT MANUALLY
-// ****** Edit ShortCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
+// ****** Edit CharCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
 //
 // @formatter:off
 package io.deephaven.engine.table.impl.updateby.minmax;
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/ShortCumMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/minmax/ShortCumMinMaxOperator.java
@@ -1,6 +1,10 @@
 //
 // Copyright (c) 2016-2025 Deephaven Data Labs and Patent Pending
 //
+// ****** AUTO-GENERATED CLASS - DO NOT EDIT MANUALLY
+// ****** Edit CharCumMinMaxOperator and run "./gradlew replicateUpdateBy" to regenerate
+//
+// @formatter:off
 package io.deephaven.engine.table.impl.updateby.minmax;
 
 import io.deephaven.base.verify.Assert;
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollinggroup/RollingGroupOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollinggroup/RollingGroupOperator.java
@@ -234,8 +234,7 @@ public RollingGroupOperator(
             @NotNull final String[] affectingColumns,
             @Nullable final String timestampColumnName,
             final long reverseWindowScaleUnits,
-            final long forwardWindowScaleUnits,
-            @NotNull final TableDefinition tableDef) {
+            final long forwardWindowScaleUnits) {
         super(pairs[0], affectingColumns, timestampColumnName, reverseWindowScaleUnits, forwardWindowScaleUnits, true);
 
         this.pairs = pairs;
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/ByteRollingMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/ByteRollingMinMaxOperator.java
@@ -168,4 +168,7 @@ public UpdateByOperator copy() {
         // endregion extra-copy-args
         );
     }
+
+    // region extra-methods
+    // endregion extra-methods
 }
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/CharRollingMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/CharRollingMinMaxOperator.java
@@ -164,4 +164,7 @@ public UpdateByOperator copy() {
         // endregion extra-copy-args
         );
     }
+
+    // region extra-methods
+    // endregion extra-methods
 }
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/DoubleRollingMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/DoubleRollingMinMaxOperator.java
@@ -168,4 +168,7 @@ public UpdateByOperator copy() {
         // endregion extra-copy-args
         );
     }
+
+    // region extra-methods
+    // endregion extra-methods
 }
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/FloatRollingMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/FloatRollingMinMaxOperator.java
@@ -168,4 +168,7 @@ public UpdateByOperator copy() {
         // endregion extra-copy-args
         );
     }
+
+    // region extra-methods
+    // endregion extra-methods
 }
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/IntRollingMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/IntRollingMinMaxOperator.java
@@ -168,4 +168,7 @@ public UpdateByOperator copy() {
         // endregion extra-copy-args
         );
     }
+
+    // region extra-methods
+    // endregion extra-methods
 }
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/LongRollingMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/LongRollingMinMaxOperator.java
@@ -7,6 +7,13 @@
 // @formatter:off
 package io.deephaven.engine.table.impl.updateby.rollingminmax;
 
+import java.time.Instant;
+import java.util.Map;
+import java.util.Collections;
+
+import io.deephaven.engine.table.ColumnSource;
+import io.deephaven.engine.table.impl.sources.ReinterpretUtils;
+
 import io.deephaven.base.ringbuffer.AggregatingLongRingBuffer;
 import io.deephaven.base.verify.Assert;
 import io.deephaven.chunk.LongChunk;
@@ -24,6 +31,7 @@ public class LongRollingMinMaxOperator extends BaseLongUpdateByOperator {
     private final boolean isMax;
     private static final int BUFFER_INITIAL_CAPACITY = 128;
     // region extra-fields
+    private final Class<?> type;
     // endregion extra-fields
 
     protected class Context extends BaseLongUpdateByOperator.Context {
@@ -147,11 +155,13 @@ public LongRollingMinMaxOperator(
             final long forwardWindowScaleUnits,
             final boolean isMax
     // region extra-constructor-args
+            ,@NotNull final Class<?> type
     // endregion extra-constructor-args
     ) {
         super(pair, affectingColumns, timestampColumnName, reverseWindowScaleUnits, forwardWindowScaleUnits, true);
         this.isMax = isMax;
         // region constructor
+        this.type = type;
         // endregion constructor
     }
 
@@ -165,7 +175,22 @@ public UpdateByOperator copy() {
                 forwardWindowScaleUnits,
                 isMax
         // region extra-copy-args
+                , type
         // endregion extra-copy-args
         );
     }
+
+    // region extra-methods
+    @NotNull
+    @Override
+    public Map<String, ColumnSource<?>> getOutputColumns() {
+        final ColumnSource<?> actualOutput;
+        if(type == Instant.class) {
+            actualOutput = ReinterpretUtils.longToInstantSource(outputSource);
+        } else {
+            actualOutput = outputSource;
+        }
+        return Collections.singletonMap(pair.leftColumn, actualOutput);
+    }
+    // endregion extra-methods
 }
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/ShortRollingMinMaxOperator.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/rollingminmax/ShortRollingMinMaxOperator.java
@@ -168,4 +168,7 @@ public UpdateByOperator copy() {
         // endregion extra-copy-args
         );
     }
+
+    // region extra-methods
+    // endregion extra-methods
 }
diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/updateby/TestCumMinMax.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/updateby/TestCumMinMax.java
diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/updateby/TestRollingMinMax.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/updateby/TestRollingMinMax.java
diff --git a/replication/static/src/main/java/io/deephaven/replicators/ReplicateUpdateBy.java b/replication/static/src/main/java/io/deephaven/replicators/ReplicateUpdateBy.java
diff --git a/table-api/src/main/java/io/deephaven/api/updateby/spec/CumMinMaxSpec.java b/table-api/src/main/java/io/deephaven/api/updateby/spec/CumMinMaxSpec.java

Original file line number	Diff line number	Diff line change
`@@ -168,4 +168,7 @@ public UpdateByOperator copy() {`
`168`	`168`	`// endregion extra-copy-args`
`169`	`169`	`);`
`170`	`170`	`}`
	`171`	`+`
	`172`	`+ // region extra-methods`
	`173`	`+ // endregion extra-methods`
`171`	`174`	`}`