-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: DH-16865: Fix spinner on empty partitions list #1953
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why my comments didn't post before
packages/iris-grid/src/IrisGrid.tsx
Outdated
} | ||
); | ||
}); | ||
const { rows } = await keyTable.getViewportData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be showing the empty table in the DHC case as well if we already know there's no partitions.
b8f31c9
to
add2766
Compare
|
||
get rowCount(): number { | ||
return 0; | ||
super(dh, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can they be? Remove TODO
// TODO: cleanup, partitionTables can be null now | ||
// assertNotNull(partitionTables); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO cleanup
// throw new Error( | ||
// `Invalid partition config set. Expected ${partitionTables.length} partitions, but got ${partitions.length}` | ||
// ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not throw here?
Closing PR since this bug has been resolved in #2110 |
Closes #1904