feat: DH-21365: DRAFT JS Immutable Table API

[niloc132]

Early draft of a proposed API for immutable tables in JS, allowing chaining table operations synchronously, letting intermediate tables be auto-released, and clearly indicating what tables should be retained for later use/disposal.

[github-actions]

No docs changes detected for ff2cfff

[niloc132]

To build the typescript and html typedocs, run :web-client-api:types:assemble

[mofojed]

The two examples provided don't make sense. There should also be examples of how to subscribe to a viewport/updating a viewport, then how to change a filter and set a new viewport. Also, how does this work with hierarchical tables, partitioned tables, etc.

Side question about the jsapi-types, why do they include Iterator and IIterableResult? We shouldn't need to define those ourselves anymore?

[mofojed]

What? This appears in the TS output:

[niloc132]

Right, its a workaround - because TS does the structural type thing, we haven't had a need for a nominal type to be actually declared that's just an intersection of two other types. For the moment we need to declare a new method in the type for the tooling to work, will get that fix and this workaround removed.

[mofojed]

I believe this comment is outdated, need to await on the resolve() method?

[niloc132]

resolve() is only needed for type safety i think in the specific case where one has a dh.TableOperations instance and wants to resolve it. If you have a dh.ResolvedTable, you can await it or wrap in PRomise.resolve and its already going to resolve to the same thing, and if you await/Promise.resolve a dh.PendingTable, since it is PromiseLike<dh.ResolvedTable>, it will resolve already to a resolved table. Technically, since TS knows both subtypes of dh.TableOperations, it might be smart enough to exhaustively check and know that awaiting any dh.TableOperations always ends with a dh.ResolvedTable, but I wasn't 100% that this would always work - this is an escape hatch to guarantee it.

[mofojed]

Is there a way to generate these docs such that they colorize the syntax correctly?

[niloc132]

I'm not sure, I'll see what typedoc wants as input to achieve that.

[mofojed]

This doesn't make sense. Should this be a = table.findColumn('A')? What is the comment above about table being "potentially invalid" mean?

[niloc132]

Close, should be result.findColumn (table is a db.TableOperations, has no data or metadata), will fix.

The await in the previous statement means that table might have been released since we didn't retain it. This concept is what lets consumers not need liveness scope or need to hold every intermediate table in a reference to close it - after the event loop is done, the API may close any pending table that wasn't retained. Since table wasn't retained, as it says, any use of methods on table may be invalid.

Of course, table could be a dh.ResolvedTable, so it might not be invalid. But in this method, we only see that it is a db.TableOperations, so we can't know that it would be safe.

If this doesn't work, we either need liveness scopes, or explicit closes on everything, or ResolvedTable can't inherit methods from TableOperations so that this mistake can't occur. This is really the crux of the "usability" argument here, if we don't think it can be clear, then we have to make one of the other choices.

[mofojed]

Ah right, and result is a ResolvedTable, so should be able to find the column there.

So something like this would be invalid?

async function process(table: dh.TableOperations) {
  const result = await table.where(...);
  const result2 = await table.sort(...);
}

And how do we fetch the table from the session? Right now we've got IdeSession#getObject which returns either a Table, TreeTable, PartitionedTable, or a Widget. Would that instead return a PendingTable?

[niloc132]

Correct, that would be potentially invalid, based on whatever the caller passed in. My intention is to be as strict as possible and make it hard to write racy code, but cases like this are hard because the API can't know at runtime what types the method declared...

Haven't solved the problem of how do we fetch from the session yet. My main idea is that dh.Table implements ResolvedTable, so you can build any operations you want from there, but you own the reference you were given. There is some ambiguity that this introduces though, like how dh.Table.selectDIstinct() returns Promise<dh.Table> instead of dh.PendingTable (no promise).

Another option could be a way to say "actually just give me the PendingTable" so that you can make calls right away without awaiting the response. That could be handy for things like newTable, so that you can use it before it finishes uploading.

[mofojed]

That sounds like a major footgun. So in the above example, result2 could throw if it was a PendingTable passed in? Can you explain why?

[niloc132]

async function process(table: dh.TableOperations) {
  const result = await table.where(...);
  const result2 = await table.sort(...);
}

First, this goes away entirely if the table is declared as dh.ResolvedTable, since you're saying "i own this, it won't go away until I say so". This problem is specific to dh.PendingTable (formerly just dh.TableOperations in the earlier examples/docs) - the client hasn't expressed that this table should live longer by retain()ing it or awaiting it, so it won't.

This is important to get right in JS more than other languages because JS serves a purpose that others don't - long lived sessions with lots of tables created and discarded along the way.

We have roughly 3 (and a half?) choices for "how does the API handle intermediate tables". We can't use GC - client memory pressure has nothing to do with server CPU usage or memory usage, and JS doesn't have a "decref" for the final reference going out of scope, so there's no way for the API to know what actual references to "immutable table" instances are live in JS and potentially might be called again. We also can't reliably replay operations in all cases - for reconnect, we can do a "best effort" at replaying operations, ending with "sorry, that couldnt reconnect" events - we don't want a table api that says "sorry, I couldn't do that, can you ask me again differently" as one of the valid choices.

The most obvious and probably wrong answer is 1) always require close() on every object that is created. We can still do the "await/resolve to get data/metdata" thing, and chain methods in lieu of awaiting, but those handles have to be cleaned up somehow. In the case above, process() would need as part of its contract "i close table when i'm done with it (so pass me a copy if you don't want that)" or "i don't close table, you must do it, but only after my operations are complete. We either lose the ability to batch, or close calls made don't actually do their release until the current event loop's batches have been all sent off, so you could legally say

table = foo();
table.close();
process(table);

which is a little silly, but no different than

table = foo();
process(table);
table.close();

since process() has a call on table after the first await. The caller could explicitly try/finally with await

table = foo()
try {
  await process(table)
} finally {
  table.close();
}

or

table = foo()
process(table).finally(_ => table.close())

Naturally this is pretty verbose and gets worse as we chain more than one method.

Java (and server-side Python) solves with with 2) liveness scopes. If you use an object in a scope and don't otherwise make sure it is owned by another liveness scope, then when that one scope goes away (is explicitly closed), the object goes away. This works great for chaining method calls, retaining the result you know you'll use, and since you didn't keep track of the others, letting the rest go. Python does this too on the server, and can even decorate a function or create a with block (try-with-resources in java, using in JS) for nice language support. These have their own foot-guns - Java defaults to an immortal scope (e.g. "you didnt make a scope, so this lives forever"), though we might want micro-task level scope to avoid leaks - but that gets you back into the footgun you're describing above, because as soon as the current microtask is up, if you didn't declare ownership of table to live longer than a microtask, that's that. Simple, unnamed, ephemeral liveness scopes are the easiest - in theory we can use a TS decorator or something, but there are still footguns - any .then() chained promise means a new function as the callback, and that needs to be tied into a scope too. Lots of different options, but had understood you to reject liveness scopes at least as part of the JS API, so I haven't built that out in any way.

Option 3) is roughly what this PR tries to build - fluent apis, uninterrupted by storing each intermediate value so it can be closed, and avoiding the need to wrap with liveness scopes. Anything not retained is ephemeral, most plausible and consistent lifetime is "this event loop" by sticking a microtask in which signals that it can no longer be used. Naturally we could make them live longer ("next 'real' task", "10 seconds", "end of the grpc call to the server"), but any arbitrary time limit is going to end up surprising someone by being faster or slower in different contexts.

Finally we could sort of sidestep the of Pending vs Resolved by just taking TableOperations away from one side or the other. This way the function could only declare PendingTable, and there's no ambiguity, you always must await a PendingTable to get a ResolvedTable, PendingTables have to follow one of the models above, and to get a PendingTable from a ResolvedTable you have to call some method on it, table.operate() to make a PendingTable, and then one of these options to make sure the PendingTable gets cleaned up. It will be more verbose, but no way to confuse lifetimes.

--

The mutable table API is meant to make all of this hard to use wrong - it was expected when we first discussed this that the immutable table api would require more explicit resource management, so might be a little harder to get right (yet more powerful).

[mofojed]

Comment unfinished.

[mofojed]

This function isn't used?

[niloc132]

Right, the class isn't used either. Hypothetically after a user creates and manipulates a SwapFilters instance, they will want data at some point, so will call await t.loadData().

[niloc132]

The two examples provided don't make sense. There should also be examples of how to subscribe to a viewport/updating a viewport, then how to change a filter and set a new viewport. Also, how does this work with hierarchical tables, partitioned tables, etc.

Can you elaborate on what doesn't make sense? Viewports/subscriptions/snapshots will follow the same pattern as the newer apis in dh.Table, same options, semantics, I'll copy over those docs as I build the impls, but since its the same, I didn't cover it yet (except to use a snapshot as a sample).

byExternal will probably look exactly as it does in dh.Table. OTOH, rollup/tree have a few options - we could make a proper immutable of rollup/tree, or we could continue to return the existing dh.TreeTable, which is mutable. THose APIs aren't reflected in other clients, and require a little more work to use the proper immutable references - no "setExpanded"/"isExpanded" apis, lots of extra columns visible, so I think we want to just continue returning wrapped/mutable instances?

Side question about the jsapi-types, why do they include Iterator and IIterableResult? We shouldn't need to define those ourselves anymore?

Definitely shouldn't, esp if PromiseLike compiles correctly without generating in the same way, I'll look into it.

[niloc132]

@mofojed here's an example of where table.resolve() could be useful - this part is wrong and fails in ts playground, the constructor either needs to take PendingTable, or this expression needs to be changed to table.resolve(). This is only applicable when we for some reason have a TableOperations instead of either subtype.

One other option here could be to not even use TableOperations at all, but always instead reference dh.PendingTable | db.ResolvedTable. That union should always resolve properly, but is a bit more verbose, and at least as a Java developer it seems silly to name two types when there's a perfectly good supertype right there.

[mofojed]

@niloc132 The examples don't make sense as in they wouldn't compile, have unused functions, don't have consistent formatting (missing semicolons at the end on some lines but having them on others), and mix usage of const and var.
I would expect an example of getting a table from the server, applying a filter to the table, listening to a viewport on the table and printing out the results, closing the table. I think it's important to do complete examples; you're saying "its the same", but - what does that mean? I would need to create a new viewport/subscription on the new table object after applying the filters, correct? Which is different than today.
Side note, looks like the irisapi filter examples are broken: https://dev-gplus.int.illumon.com:8123/irisapi/table_filter.html
Trying to right-click on Code Studio and do Equals:

Returning a existing TreeTable API which is mutable may be confusing...

[niloc132]

If you're referring to the existing HTML examples, no, I haven't touched those, and they wil be made to work as they used to. This draft is only complete enough to look at the API, the implementation (even of the changes to widen other existing types to be more consistent) is incomplete. This is only suitable for an design review - we had the shared doc before, this is actual code to make sure the typescript is plausible and usable.

The embedded JS/TS samples may likely have errors too, I've been writing them as I've been building the API as it stands, and that has changed as we've discussed. I see now that var in process... i think it was half-finished or useless autocomplete, I'll replace it - I've done a little too much copying back and forth between IJ and TS playground.

[niloc132]

This will need to also implement HasEventHandling or some other callback mechanism to facilitate size change events and failure events (via the ETUM stream). Disconnect/reconnect/reconnectfailed events would be a good idea too.

The size property should not behave like dh.Table's size, but always be accurate based on the ETUM stream rather than tied to a particular subscription. (Subscription types should provide their own sizes too, if they don't already.)