feat: a new PartitionedTableService API in Python

[jmao-denver]

Fixes #6171

Most recent nightlies run: https://github.com/nbauernfeind/deephaven-core/actions/runs/11502810543/

[chipkent]

All of the pydocs in this file are very opaque. They are the kind of docs that make sense only after understanding how everything works. They will not be clear to new users. This is probably made worse by not having a package pydoc to provide some sort of overview. I can say that my review is harder than necessary because of the docs and lack of breadcrumbs for motivation on what is going on and why.

Overall, the docs are so unclear that I quit reviewing early.

[nbauernfeind]

Your analysis of the TableDataService is correct; it's really hard to understand without being able to look at it from a higher level. Ideally, we should have a deephaven.io level documentation that describes the approach, the code/type hierarchy, and offers a simple sample.

I would not expect any form of casual user to stumble onto this API and try to use it without following some form of descriptive tutorial.

[chipkent]

In spite of all of that deflection, this package doc usability is the worst out of our entire python API. Some effort should be made to at least create a package docstring that provides enough high level view so that a user knows if they want to battle with trying to understand the rest of the docs in the package.

[rcaudy]

Caught up

[chipkent]

In spite of all of that deflection, this package doc usability is the worst out of our entire python API. Some effort should be made to at least create a package docstring that provides enough high level view so that a user knows if they want to battle with trying to understand the rest of the docs in the package.

[jmao-denver]

Just a question: I noticed that table_locations has a success_cb but table_schema and table_location_size don't. A bit inconsistent IMO.

[rcaudy]

Seems weird that errorMapper accepts an Exception, but error is a RuntimeException. Maybe we want to be more permissive in specifying error, or just get rid of errorMapper?

[nbauernfeind]

I wanted the mapper to guarantee that we can throw it without declaring that the method call throws. I wanted to be permissive on the way into the mapper so we could setError to an IOException, for example.

However, I do see that I ended up wrapping the IOException in an UncheckedDeephavenException anyway .. and then that might be wrapped in a TableDataException.

The benefit of the mapper is to capture that the error may have originated off-thread and therefore both stack traces may have real value to the user. As well as, to move specifics about the call into the mapper (for consistency in error message) and allow for the internal error to not necessarily care the origin of the request.

[nbauernfeind]

Ah, actually, the Exception entry to the helper is because of the interrupted exception in awaitResult. Will make the holder more permissible.

[rcaudy]

In a similar case for table_schema, you wrapped in an IllegalArgumentException.

[nbauernfeind]

You're right; I would rather not wrap in table_schema -- that should catch a RuntimeException and not Exception.

[nbauernfeind]

Well, technically the wrapper in table_schema specifies which schema failed to parse.. I'll clarify tomorrow.

[nbauernfeind]

Just a question: I noticed that table_locations has a success_cb but table_schema and table_location_size don't. A bit inconsistent IMO.

This because they are only allowed to call the schema_cb and size_cb one time and doing so indicates success.

[rcaudy]

.

[rcaudy]

Suggested a change to the tl sub code.

[rcaudy]

I'm ready to approve. I understand there's a bit more testing ongoing.

[deephaven-internal]

Labels indicate documentation is required. Issues for documentation have been opened:

Community: deephaven/deephaven-docs-community#353