Skip to content

feat: DH-21235: Column Restriction JS API#7956

Open
dgodinez-dh wants to merge 40 commits intodeephaven:mainfrom
dgodinez-dh:dag_ColumnRestrictionApi
Open

feat: DH-21235: Column Restriction JS API#7956
dgodinez-dh wants to merge 40 commits intodeephaven:mainfrom
dgodinez-dh:dag_ColumnRestrictionApi

Conversation

@dgodinez-dh
Copy link
Copy Markdown
Contributor

  • 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.

@dgodinez-dh dgodinez-dh requested a review from niloc132 April 28, 2026 19:35
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 28, 2026

Deploying docs previews for 61f8b90 (available for 14 days)

Python
Groovy

@dgodinez-dh dgodinez-dh added the NoReleaseNotesNeeded No release notes are needed. label Apr 28, 2026
Copy link
Copy Markdown
Contributor

@cpwright cpwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread docs/groovy/how-to-guides/input-tables.md Outdated
Comment thread docs/groovy/how-to-guides/input-tables.md Outdated
Comment thread docs/groovy/how-to-guides/input-tables.md Outdated
Comment thread docs/groovy/how-to-guides/input-tables.md Outdated
Comment thread docs/groovy/how-to-guides/input-tables.md Outdated

Deephaven provides several built-in validators:

- **RangeValidatingInputTable** - Validates that integer values fall within a specified range (min/max inclusive)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same edits to Python, but also update to snake_case

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" )

dgodinez-dh and others added 2 commits April 29, 2026 13:03
Co-authored-by: margaretkennedy <82049573+margaretkennedy@users.noreply.github.com>
@mofojed mofojed changed the title feat: DH-21235: Column Restrction JS API feat: DH-21235: Column Restriction JS API Apr 29, 2026
Copy link
Copy Markdown
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants