gRPC error: ColumnAggregations should have at least two pairs

[niloc132]

When implementing the JS API for aggregations, the consistent UI is to let the user build a list of aggregations (at least one), and so in turn the API behind the UI will take at least one aggregation and translate that list into an aggregation object to send to the server.

However, the server emits only Internal Error: Details Logged w/ID '5d9df6ae-e6b6-482e-90fe-9ae7771c20c3' when this happens. The server error is actually:

java.lang.IllegalArgumentException: class io.deephaven.api.agg.ColumnAggregations should have at least two pairs
        at io.deephaven.api.agg.ColumnAggregations.checkSize(ColumnAggregations.java:40)
        at io.deephaven.api.agg.ImmutableColumnAggregations.validate(ImmutableColumnAggregations.java:94)
        at io.deephaven.api.agg.ImmutableColumnAggregations.access$400(ImmutableColumnAggregations.java:27)
        at io.deephaven.api.agg.ImmutableColumnAggregations$Builder.build(ImmutableColumnAggregations.java:189)
        at io.deephaven.api.agg.ImmutableColumnAggregations$Builder.build(ImmutableColumnAggregations.java:123)
        at io.deephaven.api.agg.Aggregation.of(Aggregation.java:82)
        at io.deephaven.server.table.ops.AggregationAdapter.adapt(AggregationAdapter.java:71)
        at io.deephaven.server.table.ops.AggregationAdapter$Adapter.adapt(AggregationAdapter.java:231)
        at io.deephaven.server.table.ops.AggregationAdapter$Adapters.adapt(AggregationAdapter.java:100)
        at io.deephaven.server.table.ops.AggregationAdapter.adapt(AggregationAdapter.java:59)
        at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
        at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1655)
        at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
        at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
        at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
        at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
        at io.deephaven.server.hierarchicaltable.HierarchicalTableServiceGrpcImpl.lambda$rollup$1(HierarchicalTableServiceGrpcImpl.java:78)

While technically this is true, it is a lot like making Arrays.asList(...) throw an exception that you should have used Collections.singletonList(...) - your IDE's static analysis will detect this and it certainly will warn (or error, if so configured), but forcing a runtime error based on the length of the list seems silly.

As it stands, the JS client has to either force through its API that aggregations of 1 column must use a different method, or must count the elements in the collection and build output differently, instead of the trivial server unwrapping that could take place. Python almost certainly will have the same issue, and given the Java example above, I suspect any java client would reasonably object here as well.

[devinrsmith]

It looks like this aggregation is being created with zero columns. I can add a better error message. The case of 1 column should already be handled by this codepath. @niloc132

[devinrsmith]

The error message could be improved from the ColumnAggregations perspective (I'll get a quick PR out).

Take a look at io.deephaven.server.table.ops.AggregateGrpcImpl#validateRequest, I'm assuming that HierarchicalTableServiceGrpcImpl may want to take a similar approach. It eventually reaches down into io.deephaven.server.table.ops.AggregationAdapter#validate(io.deephaven.proto.backplane.grpc.Aggregation.AggregationColumns), which will ensure non-zero fields:

GrpcErrorHelper.checkRepeatedFieldNonEmpty(columns, AggregationColumns.MATCH_PAIRS_FIELD_NUMBER);

[niloc132]

It looks like this aggregation is being created with zero columns. I can add a better error message. The case of 1 column should already be handled by this codepath.

I'm able to create an agg with two columns, but not with one. See the checkSize():

        if (pairs().size() < 2) {
            throw new IllegalArgumentException(
                    String.format("%s should have at least two pairs", ColumnAggregations.class));
        }

[devinrsmith]

@niloc132, that's by design, but I think you are running into other issues with the client code. In your stacktrace, io.deephaven.api.agg.Aggregation.of(Aggregation.java:82) should be handling the 1 column case correctly.

If you want to merge in #3242, hopefully that provides a better error message to validate what I think the stacktrace is showing.

[niloc132]

I understand that it is by design, I am saying the design is bad. I'm creating this from a client in js, and suggesting that python will have the same problem, since the server could trivially handle this case (just as the java api wrapper does), but elects to instead throw an exception.

[rcaudy]

I will admit to being nearly as annoyed by this validation as @niloc132 was, although in most cases users of the Java Table API in the engine won't hit this because they create their aggregations using helper methods.

[devinrsmith]

Okay. Is this ticket purely a "bad design" ticket, or do we think there is an actual server bug? B/c from everything I can see, it looks like the client is trying to construct a request with 0 pairs (I expect 1+ pairs to work).

[niloc132]

It is looking like the mistake was mine, as I can't reproduce it now, and don't see an obvious change in this area that would have fixed this. It had appeared that I was able to reproduce this only via gRPC, but that the Java api somehow normalized this away only for Java calls (where I would have expected that this could serve as a nacent qst-driven optimization).

I do get a different error if I send zero match pairs:

io.deephaven.proto.backplane.grpc.Aggregation.AggregationColumns must have at least one match_pairs (2)
        at io.grpc.Status.asRuntimeException(Status.java:539)
        at io.grpc.protobuf.StatusProto.toStatusRuntimeException(StatusProto.java:52)
        at io.deephaven.proto.util.Exceptions.statusRuntimeException(Exceptions.java:14)
        at io.deephaven.extensions.barrage.util.GrpcUtil.statusRuntimeException(GrpcUtil.java:94)
        at io.deephaven.server.grpc.GrpcErrorHelper.checkRepeatedFieldNonEmpty(GrpcErrorHelper.java:37)
        at io.deephaven.server.table.ops.AggregationAdapter.validate(AggregationAdapter.java:65)
        at io.deephaven.server.table.ops.AggregationAdapter$Adapter.validate(AggregationAdapter.java:227)
        at io.deephaven.server.table.ops.AggregationAdapter$Adapters.validate(AggregationAdapter.java:97)
        at io.deephaven.server.table.ops.AggregationAdapter.validate(AggregationAdapter.java:56)
        at java.util.ArrayList.forEach(ArrayList.java:1541)
        at java.util.Collections$UnmodifiableCollection.forEach(Collections.java:1085)
        at io.deephaven.server.hierarchicaltable.HierarchicalTableServiceGrpcImpl.validate(HierarchicalTableServiceGrpcImpl.java:106)
        at io.deephaven.server.hierarchicaltable.HierarchicalTableServiceGrpcImpl.lambda$rollup$1(HierarchicalTableServiceGrpcImpl.java:66)

which leads me to suspect that there was at least one bug at the time I wrote the original report (ColumnAggregations should have at least two pairs), but as I can't reproduce this right now, I'll close it.