UpdateBy gRPC

[devinrsmith]

Fixes #2607

[niloc132]

👎 as it stands - this is introducing a lot (all? not sure) of QST via gRPC in this one new feature, and that means there is a lot of code that isn't obvious from the pull request which is now reachable from any client. Easiest example: a Expression.type.raw is an easy way to smuggle arbitrary java for execution, but there are no safeguards introduced to prevent this (the updateby group_by field may hold those raw expression strings, and engine-table's SelectColumn.ExpressionAdapter will invoke SelectColumnFactory.getExpression(..) on them without a second look).

At the very least, if this feature can only be implemented by using QST's Expression types, I would a) like to see the qst protos moved to a new .proto file, and b) a safe-by-default config option preventing the use of the UpdateByGrpcImpl type.

[niloc132]

IIRC not all languages qualify these with namespacing, it probably would be a good idea to give more descriptive names to avoid collisions.

[devinrsmith]

That's unfortunate :/ but good to know.

[niloc132]

convention elsewhere has been to just encode these as foo=bar/foo strings - ComboAggregateRequest.Aggregate.match_pairs for example.

i see the point that we can be more descriptive by describing it this way, but for a=a we have to write the string twice anyways, and a plain string for each of the two fields is also not accurate, since it can only be java identifiers.

[devinrsmith]

I'll add some documentation - but the idea is that input_column_name can be empty for the a=a case:

    public static Pair adapt(io.deephaven.proto.backplane.grpc.Pair pair) {
        final ColumnName output = ColumnName.of(pair.getOutputColumnName());
        return pair.getInputColumnName().isEmpty() ? output : Pair.of(ColumnName.of(pair.getInputColumnName()), output);
    }

[niloc132]

snake case for message fields (maximumLoadFactor, targetLoadFactor too)

[devinrsmith]

Good catch.

[niloc132]

why is this a oneof?

[devinrsmith]

It's explicitly modeled this way at the table api level b/c there is room for additional types in the future:

public interface UpdateByClause {
...
    interface Visitor<T> {
        T visit(ColumnUpdateClause clause);
    }

[niloc132]

Since 0 is the "This is not set" default, consider making it fail-safe with THROW, so as to not surprise users who fail to set this?

[devinrsmith]

In context, at least right now, this is always being prefaced w/ optional. The idea is that if the client does not provide a value for it, it will inherit the appropriate server defaults (the defaults vary depending on the specific field):

message UpdateByEmaOptions {
  optional BadDataBehavior on_null_value = 1;
  optional BadDataBehavior on_nan_value = 2;
  ...

I'll add documentation at this layer to explain that.

That said, I'm not against making THROW = 0, but there may be reasons to want the order to be the same as the java enum it is mapping? (We can update the java enum as well...)

[niloc132]

why only raw and long_value? if various other numbers/etc can be raw, why can't long?

Also it seems odd to refer to the "type" as being "column_name" or "raw" - perhaps expression could have a "value" which could be "column reference" or "long literal", "raw X value", etc?

[niloc132]

Just found the Expression type hierarchy in existing code, and I'm only more confused - why is RawString not a Value, but ColumnName is? Shouldn't ColumnName be a "Reference" or something rather than a Value?

[devinrsmith]

column name, long value, and raw are the only plumbed through parts at the table api layer right now, but I agree we should flesh these out more (#830). I can complete the loop a bit here in these regards.

As it is right now,

Expression = Value | RawString
Value = ColumnName | long (literal)

Maybe we should add more hierarchy here, with "Reference" and "Literal" instead of "Value"

[devinrsmith]

I've removed this proto.

[devinrsmith]

At first I was a bit confused by your comment on "easy way to smuggle arbitrary java for execution" - is there anything preventing the equivalent via a view or select? Looking at the code now though, I see io.deephaven.server.table.validation.ColumnExpressionValidator#validateColumnExpressions... it looks like this is the protection you are referring to?

I'll adapt these checks in.

I'm hoping that we can (eventually) migrate from grpc -> table api into grpc -> qst -> table api, as I think it reduces duplication and provides other benefits IMO.

[devinrsmith]

Looking deeper, I'm having trouble finding out if we disable arbitrary code execution via gRPC table operations api today. Afaict, we only do "this looks like a valid operation" validation. I would also argue, this should be the responsibility of the engine, not the grpc layer. Trying to get clarification from @rcaudy

[rcaudy]

Minor comments. It's a lot of code for not a lot of functionality.

[rcaudy]

It surprised me that you were opting to not make input column name explicit.

[rcaudy]

I don't hate this. I do sort of feel like the layering is getting excessive. I think the evidence of this can be inferred from how long it took you (@devinrsmith ) and I to trace through some of the table creation logic for parent resolution.

[devinrsmith]

Note: this PR also brings selectDistinct in-line with the security of view, update, etc (previously, it was very locked down):

[rcaudy]

Marker, I've reviewed up to here.

[devinrsmith]

I've force pushed removing any changes to TableSpec and gRPC impl via TableSpec. I haven't resolved any concerns wrt the .proto structure yet.

[niloc132]

probably not meant to be added?

[devinrsmith]

I'll remove it, and looks like I need to also fix some merge conflicts.