feat: DH-21235: Column Restriction JS API#7956
feat: DH-21235: Column Restriction JS API#7956dgodinez-dh wants to merge 40 commits intodeephaven:mainfrom
Conversation
dgodinez-dh
commented
Apr 28, 2026
- add validators (and abstract base class) to test all column restriction message types
- add ColumnRestriction to Column
- add ColumnRestrictionConverter interface to convert protobuf message to a restriction
- add a register method to WebBarrageUtils. This will be called in DHE to register a converter for Enum restrictions.
cpwright
left a comment
There was a problem hiding this comment.
I have not looked at any of the web/client-api changes.
|
|
||
| @Override | ||
| public @Nullable List<Any> getColumnRestrictions(String columnName) { | ||
| final List<Any> columnRestrictions = wrapped.getColumnRestrictions(columnName); |
There was a problem hiding this comment.
We've created a lot of these validators. At this point the volume of them seem to be indicating that these might actually be intended for use and should be documented. If they are really for test use only, why do we need to have 5 of them?
As such we might also want to reduce the code duplication by having an abstract method in our base that does the general wrapping and merging of new restrictions.
There was a problem hiding this comment.
I created 5 so that I could test that all restriction messages in inputtable.proto could be converted to my ColumnRestriction class.
I have:
- added an abstract method to wrap an updater
- added python and groovy documentation for these classes.
There was a problem hiding this comment.
I think the real issue is how is this going to look in core+'s js-client project, where we have EnumRestriction with its associated lists. The JS api needs some kind of "Here's how you plug in another proto type that wasn't defined in DHC's js or proto", and the dh.ColumnRestriction type needs a way to ask "What restrictions do you have. Maybe the js-client project registers new marshallers for this that are exposed directly to JS consumers, or maybe ColumnRestriction itself needs to be more abstract and expose arbitrary data rather than ranges and lists? Exposing type there is almost enough, but the api in this PR seems to say there are only five values rather than "This is unbounded, and if your data doesn't fit here, its up to the consumer to cast as needed."
Maybe this would be easier to follow if there was a draft of the DHE change that depends on this?
There was a problem hiding this comment.
I have a prototype branch where I tested this our in Core+: https://github.com/deephaven-ent/iris/tree/dag_EnumRestrictionApi
There is one issue I ran into, which is that I could not find a good place to call registerColumnRestrictionConverter. I ended up calling in the PivotTableService, which is not meant to be a long term solution but just a way to test if it works.
With regards to how I intended it to work:
- Core+ registers it's own converter that fits it's data into the existing dh.ColumnRestriction.
- The dh.ColumnRestriction will show up in the Column in the js client, which will use that data to inform the input table UI (this will happen in
web-client-ui.
There was a problem hiding this comment.
I thought I replied to this, but it seems to not be here...
Declare an EntryPoint type in core+, and mark it as in the .gwt.xml file for core+, and it will run at startup.
Even with this though, you still are preventing any downstream validation mechanism from being able to add any restriction that isn't "it can/can't be null, heres a list of strings, a max, and a min".
Instead, my quick thought:
- Let the column declare a validator, rather than abstract list of what restrictions we can imagine today. Then, we can register a parser and a validator to go with it, that will take a value and check if it matches the restrictions, returning a helpful error message.
- Let the column hold a map/array of restrictions, each with a type string and its own structure, rather than trying to build a one-size-fits-all up front. If the server then supports something that the client doesn't understand, that's okay - the server will still catch it, but when the client recognizes the restriction types it can more proactively help guide the user towards valid values ("Column Foo must be greater than column Bar", "Size must be -1 or greater than 100", etc).
There was a problem hiding this comment.
Ok, I've made some updates:
- Column has an array of ColumnRestiction
- ColumnRestriction is a type and map of data
- Added an optional validator type that can be registered with the converter
- Updated my Core+ branch to test these changes
|
|
||
| Deephaven provides several built-in validators: | ||
|
|
||
| - **RangeValidatingInputTable** - Validates that integer values fall within a specified range (min/max inclusive) |
There was a problem hiding this comment.
Same edits to Python, but also update to snake_case
There was a problem hiding this comment.
I updated style in the python doc to match groovy for this list. But I'm not sure we should change to snake_case. The python code refers to these class using the Java camel case:
range_validating_input_table = jpy.get_type( "io.deephaven.server.table.inputtables.RangeValidatingInputTable" )
Co-authored-by: margaretkennedy <82049573+margaretkennedy@users.noreply.github.com>
niloc132
left a comment
There was a problem hiding this comment.
Added a reply that apparently got lost from a review last week - also was discussed in slack briefly - I don't think this is flexible enough to be future proofed.