fix: DH-16865: Fix spinner on empty partitions list #1953
Conversation
vbabich
changed the title
|
This PR should fix the issue with partitioned tables with no partitions in DHE. But I think I found an edge case with widgets of type |
packages/iris-grid/src/IrisGrid.tsx
Outdated
| } | ||
| ); | ||
| }); | ||
| const { rows } = await keyTable.getViewportData(); |
There was a problem hiding this comment.
We shouldn't be returning the initial partition until it's available. Right now this is getting stuck in a loop, as it's trying to apply a partition and one doesn't exist:
from deephaven import empty_table, time_table
t = empty_table(0).update("X=ii").partition_by("X")
t2 = time_table("PT20S").update("X=ii").where("X>0").partition_by("X")
There was a problem hiding this comment.
Yep, I only tested with DHE partitioned tables, didn't realize PARTITIONEDTABLE in DHC triggers a different code path. Will fix and make sure to test both DHE and DHC.
There was a problem hiding this comment.
We shouldn't be returning the initial partition until it's available.
What should we return in that case? Ok to show the Spinner on DHC partitioned tables? In Enterprise we want to show an empty table when there's no available partitions.
There was a problem hiding this comment.
We should be showing the empty table in the DHC case as well if we already know there's no partitions.
vbabich
changed the title
vbabich
force-pushed
the
fix_null_partition_filter
branch
from
b8f31c9
to
add2766
Compare
|
|
||
| get rowCount(): number { | ||
| return 0; | ||
| super(dh, []); |
There was a problem hiding this comment.
Need to pass the formatter through.
| super(dh, []); | |
| super(dh, [], formatter); |
That being said, it's not like this really needs a formatter since it's an empty grid, but 🤷
| this.startListening(model); | ||
| this.startListeningForPartitionChanges(model); |
There was a problem hiding this comment.
I'd probably break down the current startListening/stopListening methods, e.g.
startListening(model: IrisGridModel): void {
this.startListeningForEvents(model);
this.startListeningForPartitionChanges(model);
}
As you're calling both startListening methods everywhere, may as well combine it into just one top level one.
| model: IrisGridModel, | ||
| resetState = false | ||
| ): void { | ||
| const { keyTable } = this.state; |
There was a problem hiding this comment.
There is a race condition here - in startListeningForPartitionChanges, we fetch the keyTable asynchronously, so in theory it's possible that stopListening is called before startListening completes/can set the key table.
May need to keep that promise around explicitly such that it can be cancelled/unset if stopListening is called.
| // TODO: find out how partitionTables can be null after this | ||
|
|
| // TODO: cleanup, partitionTables can be null now | ||
| // assertNotNull(partitionTables); | ||
|
|
| // throw new Error( | ||
| // `Invalid partition config set. Expected ${partitionTables.length} partitions, but got ${partitions.length}` | ||
| // ); |
Closes #1904