Skip to content

Commit f6ea293

Browse files
lbooker42claude
lbooker42
and
claude
authored
fix: DH-22381: Correct guard logic in adaptEmOptions for EM aggregations (#7938)
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 9d06228 commit f6ea293

2 files changed

Lines changed: 177 additions & 2 deletions

File tree

‎server/src/main/java/io/deephaven/server/table/ops/UpdateByGrpcImpl.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -365,10 +365,10 @@ private static EmStdSpec adaptEmStd(UpdateByEmStd emStd) {
365365

366366
private static OperationControl adaptEmOptions(UpdateByEmOptions options) {
367367
final OperationControl.Builder builder = OperationControl.builder();
368-
if (options.getOnNanValue() == BAD_DATA_BEHAVIOR_NOT_SPECIFIED) {
368+
if (options.getOnNullValue() != BAD_DATA_BEHAVIOR_NOT_SPECIFIED) {
369369
builder.onNullValue(adaptBadDataBehavior(options.getOnNullValue()));
370370
}
371-
if (options.getOnNanValue() == BAD_DATA_BEHAVIOR_NOT_SPECIFIED) {
371+
if (options.getOnNanValue() != BAD_DATA_BEHAVIOR_NOT_SPECIFIED) {
372372
builder.onNanValue(adaptBadDataBehavior(options.getOnNanValue()));
373373
}
374374
if (options.hasBigValueContext()) {

‎server/src/test/java/io/deephaven/server/table/ops/UpdateByEmOptionsGrpcTest.java‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
//
2+
// Copyright (c) 2016-2026 Deephaven Data Labs and Patent Pending
3+
//
4+
package io.deephaven.server.table.ops;
5+
6+
import io.deephaven.engine.util.TableTools;
7+
import io.deephaven.proto.backplane.grpc.BadDataBehavior;
8+
import io.deephaven.proto.backplane.grpc.ExportedTableCreationResponse;
9+
import io.deephaven.proto.backplane.grpc.TableReference;
10+
import io.deephaven.proto.backplane.grpc.UpdateByEmOptions;
11+
import io.deephaven.proto.backplane.grpc.UpdateByRequest;
12+
import io.deephaven.proto.backplane.grpc.UpdateByRequest.UpdateByOperation;
13+
import io.deephaven.proto.backplane.grpc.UpdateByRequest.UpdateByOperation.UpdateByColumn;
14+
import io.deephaven.proto.backplane.grpc.UpdateByRequest.UpdateByOperation.UpdateByColumn.UpdateBySpec;
15+
import io.deephaven.proto.backplane.grpc.UpdateByRequest.UpdateByOperation.UpdateByColumn.UpdateBySpec.UpdateByEma;
16+
import io.deephaven.proto.backplane.grpc.UpdateByRequest.UpdateByOperation.UpdateByColumn.UpdateBySpec.UpdateByEmStd;
17+
import io.deephaven.proto.backplane.grpc.UpdateByWindowScale;
18+
import io.deephaven.proto.backplane.grpc.UpdateByWindowScale.UpdateByWindowTicks;
19+
import io.deephaven.proto.util.ExportTicketHelper;
20+
import io.grpc.StatusRuntimeException;
21+
import org.junit.Test;
22+
23+
import static org.assertj.core.api.Assertions.assertThat;
24+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
25+
26+
/**
27+
* Tests for the on_null_value / on_nan_value option handling in adaptEmOptions. The original code had two bugs: the
28+
* null-value guard read getOnNanValue() instead of getOnNullValue(), and both guards used == (apply when NOT_SPECIFIED)
29+
* instead of != (apply when specified).
30+
*/
31+
public class UpdateByEmOptionsGrpcTest extends GrpcTableOperationTestBase<UpdateByRequest> {
32+
33+
@Override
34+
public ExportedTableCreationResponse send(UpdateByRequest request) {
35+
return channel().tableBlocking().updateBy(request);
36+
}
37+
38+
// -----------------------------------------------------------------------
39+
// helpers
40+
// -----------------------------------------------------------------------
41+
42+
private static UpdateByWindowScale ticksScale(double ticks) {
43+
return UpdateByWindowScale.newBuilder()
44+
.setTicks(UpdateByWindowTicks.newBuilder().setTicks(ticks).build())
45+
.build();
46+
}
47+
48+
private static UpdateByOperation emaOperation(UpdateByEmOptions options, String column) {
49+
UpdateByEma.Builder ema = UpdateByEma.newBuilder().setWindowScale(ticksScale(5));
50+
if (options != null) {
51+
ema.setOptions(options);
52+
}
53+
return UpdateByOperation.newBuilder()
54+
.setColumn(UpdateByColumn.newBuilder()
55+
.setSpec(UpdateBySpec.newBuilder().setEma(ema).build())
56+
.addMatchPairs(column + "=Value")
57+
.build())
58+
.build();
59+
}
60+
61+
private static UpdateByOperation emStdOperation(UpdateByEmOptions options, String column) {
62+
UpdateByEmStd.Builder emStd = UpdateByEmStd.newBuilder().setWindowScale(ticksScale(5));
63+
if (options != null) {
64+
emStd.setOptions(options);
65+
}
66+
return UpdateByOperation.newBuilder()
67+
.setColumn(UpdateByColumn.newBuilder()
68+
.setSpec(UpdateBySpec.newBuilder().setEmStd(emStd).build())
69+
.addMatchPairs(column + "=Value")
70+
.build())
71+
.build();
72+
}
73+
74+
private UpdateByRequest buildRequest(int exportId, TableReference sourceRef, UpdateByOperation op) {
75+
return UpdateByRequest.newBuilder()
76+
.setResultId(ExportTicketHelper.wrapExportIdInTicket(exportId))
77+
.setSourceId(sourceRef)
78+
.addOperations(op)
79+
.build();
80+
}
81+
82+
// -----------------------------------------------------------------------
83+
// Bug A: null guard read getOnNanValue() instead of getOnNullValue()
84+
// -----------------------------------------------------------------------
85+
86+
/**
87+
* on_null_value set, on_nan_value absent (NOT_SPECIFIED). Old null guard: getOnNanValue()==NOT_SPECIFIED → true →
88+
* nan branch also fires → adaptBadDataBehavior(NOT_SPECIFIED) → IllegalArgumentException.
89+
*/
90+
@Test
91+
public void emaWithOnlyOnNullValue() {
92+
final TableReference ref =
93+
ref(TableTools.emptyTable(10).update("Value = (double) ii"));
94+
final UpdateByEmOptions options = UpdateByEmOptions.newBuilder()
95+
.setOnNullValue(BadDataBehavior.SKIP)
96+
.build();
97+
final ExportedTableCreationResponse response =
98+
send(buildRequest(1, ref, emaOperation(options, "Ema")));
99+
assertThat(response.getSuccess()).isTrue();
100+
release(response);
101+
}
102+
103+
@Test
104+
public void emStdWithOnlyOnNullValue() {
105+
final TableReference ref =
106+
ref(TableTools.emptyTable(10).update("Value = (double) ii"));
107+
final UpdateByEmOptions options = UpdateByEmOptions.newBuilder()
108+
.setOnNullValue(BadDataBehavior.SKIP)
109+
.build();
110+
final ExportedTableCreationResponse response =
111+
send(buildRequest(1, ref, emStdOperation(options, "EmStd")));
112+
assertThat(response.getSuccess()).isTrue();
113+
release(response);
114+
}
115+
116+
/**
117+
* Empty UpdateByEmOptions message (hasOptions=true, all fields NOT_SPECIFIED). Old guards both fired →
118+
* adaptBadDataBehavior(NOT_SPECIFIED) → IllegalArgumentException.
119+
*/
120+
@Test
121+
public void emaWithEmptyOptions() {
122+
final TableReference ref =
123+
ref(TableTools.emptyTable(10).update("Value = (double) ii"));
124+
final ExportedTableCreationResponse response =
125+
send(buildRequest(1, ref, emaOperation(UpdateByEmOptions.getDefaultInstance(), "Ema")));
126+
assertThat(response.getSuccess()).isTrue();
127+
release(response);
128+
}
129+
130+
@Test
131+
public void emStdWithEmptyOptions() {
132+
final TableReference ref =
133+
ref(TableTools.emptyTable(10).update("Value = (double) ii"));
134+
final ExportedTableCreationResponse response =
135+
send(buildRequest(1, ref, emStdOperation(UpdateByEmOptions.getDefaultInstance(), "EmStd")));
136+
assertThat(response.getSuccess()).isTrue();
137+
release(response);
138+
}
139+
140+
/**
141+
* Regression for Bug A when both options are set: old null guard used getOnNanValue(), so with onNanValue=SKIP
142+
* (non-NOT_SPECIFIED) the guard was false and onNullValue=THROW was silently dropped. The operation then succeeded
143+
* on null input instead of throwing.
144+
*/
145+
@Test
146+
public void emaThrowsOnNullValueWhenBothOptionsSetAndNullInput() {
147+
final TableReference ref = ref(TableTools.emptyTable(3)
148+
.update("Value = (double) io.deephaven.util.QueryConstants.NULL_DOUBLE"));
149+
assertThatThrownBy(() -> send(buildRequest(1, ref,
150+
emaOperation(UpdateByEmOptions.newBuilder()
151+
.setOnNullValue(BadDataBehavior.THROW)
152+
.setOnNanValue(BadDataBehavior.SKIP)
153+
.build(), "Ema"))))
154+
.isInstanceOf(StatusRuntimeException.class);
155+
}
156+
157+
// -----------------------------------------------------------------------
158+
// Bug B: nan guard used == instead of !=, silently dropping explicit values
159+
// -----------------------------------------------------------------------
160+
161+
/**
162+
* Regression for Bug B: old nan guard getOnNanValue()==NOT_SPECIFIED was false when onNanValue=THROW, so THROW was
163+
* silently dropped and the default SKIP applied. The operation then succeeded on NaN input instead of throwing.
164+
*/
165+
@Test
166+
public void emaThrowsOnNanValueWhenOnNanValueIsThrow() {
167+
final TableReference ref =
168+
ref(TableTools.emptyTable(3).update("Value = Double.NaN"));
169+
assertThatThrownBy(() -> send(buildRequest(1, ref,
170+
emaOperation(UpdateByEmOptions.newBuilder()
171+
.setOnNanValue(BadDataBehavior.THROW)
172+
.build(), "Ema"))))
173+
.isInstanceOf(StatusRuntimeException.class);
174+
}
175+
}

0 commit comments

Comments
 (0)
⚡