Wrap the rollup and tree Table operations #3259
Conversation
jmao-denver
added
DocumentationNeeded
python-server-side
ReleaseNotesNeeded
| @@ -214,6 +218,42 @@ def _td_to_columns(table_definition): | |||
| return cols | |||
|
|
|||
|
|
|||
| class RollupTable(JObjectWrapper): | |||
There was a problem hiding this comment.
It seems like this should have some methods to be useful for something beyond the GUI.
There was a problem hiding this comment.
Yes but that effort is quite substantial and that's why this PR only partially addresses the ticket.
There was a problem hiding this comment.
Indeed. That can be a January deliverable, instead of a December one.
There was a problem hiding this comment.
Create a separate ticket and item in the planning doc.
There was a problem hiding this comment.
The ticket will stay open as this PR only partially fixes it.
| def __init__(self, j_rollup_table: jpy.JType, aggs: Sequence[Aggregation], include_constituents: bool, | ||
| by: Sequence[str]): |
There was a problem hiding this comment.
This constructor takes a rollup table and arguments that are contained in the rollup table.
- It is possible that the rollup table and the args get out of sync, which is bad.
- If users would ever touch this, we would just want them to provide the rollup table as an arg, so that we know everything is consistent.
There was a problem hiding this comment.
- I assume the args are just static properties on the RollupTable, not sure how they would change out of sync, @rcaudy ?
- That's quite unlikely as they are for visualization only.
There was a problem hiding this comment.
They are immutable properties of the RollupTable. This is safe.
There was a problem hiding this comment.
I'm just saying that what is here has the potential of being brittle -- not that it isn't correct.
| def j_object(self) -> jpy.JType: | ||
| return self.j_tree_table | ||
|
|
||
| def __init__(self, j_tree_table: jpy.JType, id_col: str, parent_col: str): |
There was a problem hiding this comment.
Same comments as the rollup table constructor.
| Note some aggregations can not be used in creating a rollup tables, these include: group, partition, median, | ||
| pct, weighted_avg |
There was a problem hiding this comment.
What makes each of these aggs not work? (e.g. median, pct, and weighed_avg)
There was a problem hiding this comment.
@rcaudy Maybe you can explain?
There was a problem hiding this comment.
We don't have the code to re-aggregate them, or in some cases a model for how it should work. This is a server-side functionality gap that is many years old and won't be addressed under the current tickets.
There was a problem hiding this comment.
Should we then have a note that is less specific, with the hope that these things do get fixed? Then we don't have to remember that these notes need to be updated.
There was a problem hiding this comment.
I'm not sure. This is Python specific messaging; the Java code has its own, right down in the authoritative code.
There was a problem hiding this comment.
I'm not sure. This is Python specific messaging; the Java code has its own, right down in the authoritative code.
There was a problem hiding this comment.
I am OK with leaving it as is because the individual failure tests will catch the change if we do get these fixed in the future.
Co-authored-by: Chip Kent <[email protected]>
|
Labels indicate documentation is required. Issues for documentation have been opened: How-to: https://github.com/deephaven/deephaven.io/issues/2002 |
The new RollupTable and TreeTable classes are very hollow right now in that no further operations are supported on them. However they abide by the JObjectWrapper protocol and should be easily made visible through DHC's gRPC API in the WebUI.
Partial #3249