fix: Allow LayoutHints to be accessible from Python, TreeTables by mofojed · Pull Request #5543 · deephaven/deephaven-core

mofojed · 2024-05-29T14:43:15Z

Related to Enterprise ticket DH-17076, which has been identified as a Blocker by support
Groovy had setLayoutHints exposed on TreeTables, but was not exposed through JsTreeTable and was also not accessible through Python
Added layoutHints to JsTreeTable, and exposed layout_hints through Python (they were already accessible through Groovy)
Will have a follow up UI change as well to read the layout hints correctly from a JsTreeTable
Fixes Expose layout_hints on TreeTable, RollupTable in Python #5554

Tested with the following snippet:

# For RollupTable
from deephaven import empty_table, agg
source = empty_table(100).update(["K1 = ii", "K2=K1%10", "Y=randomDouble(0, 10)", "Z=randomDouble(0, 10)"])
result_norollup = source.layout_hints(column_groups=[{"name": "MyCols", "children": ["Y", "Z"]}])
result_rollup = source.rollup([agg.avg(["Y", "Z"])], "K2", False).layout_hints(column_groups=[{"name": "MyCols", "children": ["Y", "Z"]}])

# For TreeTable
from deephaven.constants import NULL_INT
from deephaven import empty_table

source2 = empty_table(100).update_view(
    ["ID = i", "Parent = i == 0 ? NULL_INT : (int)(i / 4)", "X=Math.sin(i)", "Y=Math.cos(i)"]
)

result2_tree = source2.tree(id_col="ID", parent_col="Parent").layout_hints(column_groups=[{"name": "Trig", "children": ["X", "Y"]}])

- Related to Enterprise ticket DH-17076, which has been identified as a Blocker by support - Groovy had setLayoutHints exposed on TreeTables, but was not exposed through JsTreeTable and was also not accessible through Python - Added layoutHints to JsTreeTable, and exposed layout_hints through Python - Will have a follow up UI change as well to read the layout hints correctly from a JsTreeTable

- Related to DH-17076 - Needed a Core patch: deephaven/deephaven-core#5543 - Tested using both the Python and Groovy snippets from the ticket - Fixes deephaven#2035

jmao-denver · 2024-05-30T14:53:26Z

+                     column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:
+        """ Sets layout hints on the Table


Should it be a TreeTable or RollupTable? Either way, some of the typehints/docstring/messages need to reflect that.

Yes true - I should also add to TreeTable as well, not just rollup...

jmao-denver · 2024-05-30T17:14:05Z


+    def layout_hints(self, front: Union[str, List[str]] = None, back: Union[str, List[str]] = None,
+                     freeze: Union[str, List[str]] = None, hide: Union[str, List[str]] = None,
+                     column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:


Suggested change

column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:

column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> RollupTable:

jmao-denver · 2024-05-30T17:14:37Z


+    def layout_hints(self, front: Union[str, List[str]] = None, back: Union[str, List[str]] = None,
+                     freeze: Union[str, List[str]] = None, hide: Union[str, List[str]] = None,
+                     column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:


Suggested change

column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:

column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> TreeTable:

jmao-denver · 2024-05-30T17:15:55Z

Do we need some basic test cases?

chipkent

Reviewed Python

chipkent · 2024-05-31T14:44:05Z

+        Args:
+            front (Union[str, List[str]]): the columns to show at the front.
+            back (Union[str, List[str]]): the columns to show at the back.
+            freeze (Union[str, List[str]]): the columns to freeze to the front.


This leaves a lot of questions for me:

Is this separate from front?

Do the frozen columns appear before the front columns?

Do the frozen columns need to be listed in the front columns?

I just copied these from the existing layout_hints method that is already on Table. Should I make updates there as well?
I'm just trying to get this supposed Enterprise blocker completed. Didn't expect/need to refactor a whole bunch of APIs, as this is supposed to be a hotfix - trying to change as little as possible, while just exposing this functionality in Python (though they did only ask about Groovy...)

chipkent · 2024-05-31T14:44:55Z


+    def layout_hints(self, front: Union[str, List[str]] = None, back: Union[str, List[str]] = None,
+                     freeze: Union[str, List[str]] = None, hide: Union[str, List[str]] = None,
+                     column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:


dict isn't a great type hint because it doesn't indicate the contents of the dict. It should include the acceptable key and value types.

chipkent · 2024-05-31T14:45:18Z

+            freeze (Union[str, List[str]]): the columns to freeze to the front.
+                These will not be affected by horizontal scrolling.
+            hide (Union[str, List[str]]): the columns to hide.
+            column_groups (List[Dict]): A list of dicts specifying which columns should be grouped in the UI.


See typehint comment above.

chipkent · 2024-05-31T14:45:53Z

+            freeze (Union[str, List[str]]): the columns to freeze to the front.
+                These will not be affected by horizontal scrolling.
+            hide (Union[str, List[str]]): the columns to hide.
+            column_groups (List[Dict]): A list of dicts specifying which columns should be grouped in the UI.


What does "grouped in the UI" mean?

chipkent · 2024-05-31T14:47:23Z

+                The dicts can specify the following:
+
+                * name (str): The group name
+                * children (List[str]): The column names in the group
+                * color (Optional[str]): The hex color string or Deephaven color name


This is very unclear. e.g. is name the dict key? are children and color values? etc.

chipkent · 2024-05-31T14:48:42Z

+    def layout_hints(self, front: Union[str, List[str]] = None, back: Union[str, List[str]] = None,
+                     freeze: Union[str, List[str]] = None, hide: Union[str, List[str]] = None,
+                     column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:
+        """ Sets layout hints on the RollupTable


Pydocs do not mention the default behaviors.

chipkent · 2024-05-31T14:50:48Z

+                    _j_layout_hint_builder.columnGroup(group.get("name"), j_array_list(group.get("children")),
+                                                       group.get("color", ""))


This should probably error if there are other keys in the dict. e.g. a misspelling.

chipkent · 2024-05-31T14:51:46Z

        except Exception as e:
            raise DHError(e, "with_filters operation on TreeTable failed.") from e

+    def layout_hints(self, front: Union[str, List[str]] = None, back: Union[str, List[str]] = None,


apply other comments here

chipkent · 2024-05-31T14:53:02Z

+                             id_col=self.id_col, parent_col=self.parent_col)
+        except Exception as e:
+            raise DHError(e, "failed to set layout hints on tree table") from e



Should PartitionedTable or PartitionedTableProxy also have this method?

Those classes don't extend GridAttributes which has the setLayoutHints method defined in Java.
Seems like ticket #2890 (comment) is related

mofojed · 2024-06-02T19:38:52Z

I'm going to refactor the JS API part out of this as that's all that's technically required for DH-17076. I've opened a new ticket #5554 to address the inconsistency between Python/Groovy implementations of layout hints, and linked this PR as a fix, but I'm not going to continue work on this PR.
I've opened PR #5555 to address just the JS API portion, which is required for DH-17076 to be resolved.

mofojed added NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels May 29, 2024

mofojed self-assigned this May 29, 2024

mofojed requested review from chipkent, jmao-denver and rcaudy as code owners May 29, 2024 14:43

mofojed mentioned this pull request May 29, 2024

fix: DH-17076 LayoutHints on TreeTables were not being applied deephaven/web-client-ui#2041

Merged

mofojed requested a review from niloc132 May 29, 2024 21:44

niloc132 previously approved these changes May 30, 2024

View reviewed changes

jmao-denver reviewed May 30, 2024

View reviewed changes

Add support for TreeTable as well

6bff70c

mofojed dismissed niloc132’s stale review via 6bff70c May 30, 2024 16:57

mofojed requested a review from jmao-denver May 30, 2024 16:57

jmao-denver reviewed May 30, 2024

View reviewed changes

chipkent reviewed May 31, 2024

View reviewed changes

mofojed mentioned this pull request Jun 2, 2024

Expose layout_hints on TreeTable, RollupTable in Python #5554

Open

mofojed closed this Jun 2, 2024

github-actions Bot locked and limited conversation to collaborators Jun 2, 2024

		column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:
		""" Sets layout hints on the Table

		_j_layout_hint_builder.columnGroup(group.get("name"), j_array_list(group.get("children")),
		group.get("color", ""))

Conversation

mofojed commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmao-denver May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmao-denver commented May 30, 2024

Uh oh!

chipkent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mofojed Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mofojed commented Jun 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mofojed commented May 29, 2024 •

edited

Loading

jmao-denver May 30, 2024 •

edited

Loading

mofojed Jun 2, 2024 •

edited

Loading