feat: DH-21235: Column Restriction JS API#7956
feat: DH-21235: Column Restriction JS API#7956dgodinez-dh wants to merge 37 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.
|
|
||
| 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>