feat!: Add TableDefinition wrapper for python #5892

devinrsmith · 2024-08-03T00:08:51Z

deephaven.table.TableDefinition: new python wrapper for io.deephaven.engine.table.TableDefinition
deephaven.column.ColumnDefinition: new python wrapper for io.deephaven.engine.table.ColumnDefinition
deephaven.table.TableDefinitionLike: new type alias to allow for consistent public APIs and conversions into deephaven.table.TableDefinition
deephaven.column.Column: deprecated for removal
deephaven.jcompat.j_table_definition: deprecated for removal
deephaven.stream.kafka.consumer.json_spec: cols_defs specified as List[Tuple[str, DType]] deprecated for removal

BREAKING CHANGE: deephaven.column.InputColumn no longer inherits from deephaven.column.Column; as such, it no longer exposes Column's attributes. This is unlikely to affect users as InputColumn is really a structure meant to support new_table.

devinrsmith · 2024-08-03T00:09:09Z

Sketch of impl. Manual testing works. Need to add real tests.

…tion

jmao-denver · 2024-08-06T17:26:13Z

Any use cases to show that the new wrapper is better than a simple new property column_map?

devinrsmith · 2024-08-06T17:56:19Z

Any use cases to show that the new wrapper is better than a simple new property column_map?

Good Q. The advantage to this approach:

Can call java methods using TableDefinition.j_table_definition without needing to re-construct a TableDefinition
Can co-locate common methods one might make w/ TableDefinition, ie TableDefinition.table
Could introduce pythonic methods that match existing java helpers like io.deephaven.engine.table.TableDefinition#checkMutualCompatibility(io.deephaven.engine.table.TableDefinition) or io.deephaven.engine.table.TableDefinition#checkHasColumn(java.lang.String, java.lang.Class<?>).

I think we could ask the same question about the java side; why don't we use Map<String, ColumnDefinition<?>> everywhere instead of TableDefinition? Because there is good advantage to modelling it as a more specific object to provide common utility around it.

chipkent

Minor docs comment, but I'm otherwise not opposed.

py/server/deephaven/table.py

… cached_property.

…e-definition

rcaudy

I think the TableDefinition wrapper needs work, mostly re: ordering.

rcaudy · 2024-08-13T14:54:52Z

py/server/deephaven/column.py

+
+    j_object_type = _JColumnDefinition
+
+    def __init__(


We need a constructor that directly wraps an existing _JColumnDefinition.

This includes that case:

Column(j_column_definition=...)

And we use this construction as the basis for TableDefinition as a Mapping[str, Column].

It looks funky because we need to maintain compatibility with the existing constructor.

We just agreed on a call to introduce a superclass called ColumnDefinition, maybe, in order to preserve the existing behavior and also provide the kind of wrapper we want users to use.

rcaudy · 2024-08-13T14:55:43Z

py/server/deephaven/column.py

+
+    j_object_type = _JColumn
+
+    def __init__(


Ditto, this is supported via j_column_definition

We said this may not actually be a wrapper. I'm not clear that it's a wrapper around a JColumnDefinition, regardless.

py/server/deephaven/table.py

rcaudy · 2024-08-13T15:07:27Z

py/server/deephaven/table.py

+    @cached_property
    def is_refreshing(self) -> bool:
        """Whether this table is refreshing."""
-        if self._is_refreshing is None:
-            self._is_refreshing = self.j_table.isRefreshing()
-        return self._is_refreshing
+        return self.j_table.isRefreshing()

-    @property
+    @cached_property
    def is_blink(self) -> bool:
        """Whether this table is a blink table."""
        return _JBlinkTableTools.isBlink(self.j_table)

-    @property
+    @cached_property
    def update_graph(self) -> UpdateGraph:
        """The update graph of the table."""
-        if self._update_graph is None:
-            self._update_graph = UpdateGraph(self.j_table.getUpdateGraph())
-        return self._update_graph
+        return UpdateGraph(self.j_table.getUpdateGraph())

-    @property
+    @cached_property
    def is_flat(self) -> bool:
        """Whether this table is guaranteed to be flat, i.e. its row set will be from 0 to number of rows - 1."""
-        if self._is_flat is None:
-            self._is_flat = self.j_table.isFlat()
-        return self._is_flat
+        return self.j_table.isFlat()


isFlat is technically mutable, as is isRefreshing. That said, a Python wrapper should never see a table that hasn't had these set in their final form, so I don't know that it actually matters.

isBlink is effectively immutable, on the other hand; that is, once you look at it, it can't be changed (like anything else stored in attributes). updateGraph is final, also OK.

If there is a possibility for mutation, I would remove the caching because we'll never remember to make a change here if we do get to a case where mutation happens. I also doubt that these get called enough to make a significant performance difference.

I'm convinced the caching is fine. But I also agree that we are likely not calling these anywhere performance critical.

py/server/deephaven/table.py

jmao-denver

some concerns about the use of mutable @cached_property but can accept the trade-off
there seems to be a small breaking change

jmao-denver · 2024-08-13T15:16:06Z

py/server/deephaven/column.py

+    @property
+    def j_column_header(self):
+        return _JColumnHeader.of(self.name, self.data_type.qst_type)


If we don't need to worry about the mutability of @cached_property, should we use that here too?

This seems like more of an internal property and only used when creating an InputColumn; I don't think we expect users to call j_column_header? I would be okay removing it or forcing it to start w/ _, and I think tracing through the callstack, it doesn't make sense to cache it (I don't think we'll ever re-use it).

Where we might conceivably re-use it in the future is if we had a helper from Column -> InputColumn. IE,

my_col : InputColumn = Column("Foo", dtypes.int32).with_data([1, 2, 3])

But at least with the current interfaces, users are always going through InputColumn constructor or helper methods (bool_col, byte_col, etc).

jmao-denver · 2024-08-13T15:18:32Z

py/server/deephaven/table.py

+    def j_object(self) -> jpy.JType:
+        return self.j_table_definition
+
+    @property


@cached_property?

It's not clear to me if we want to cache Table; this would mean that the table would exist until TableDefinition is GCd.

I concur with Devin.

py/server/tests/test_column.py

!2, 3.9 /home/runner/work/deephaven-core/deephaven-core/py/server/deephaven/table.py builtin generic type annotation (collections.abc.Iterable[..]) requires !2, 3.9 builtin generic type annotation (collections.abc.Mapping[..]) requires !2, 3.9 Minimum required versions: 3.9 Incompatible versions: 2 Target versions not met: 3.8

py/server/deephaven/column.py

py/server/deephaven/numpy.py

py/server/deephaven/pandas.py

chipkent · 2024-08-14T16:36:25Z

py/server/deephaven/stream/table_publisher.py

@@ -75,7 +73,7 @@ def publish_failure(self, failure: Exception) -> None:

 def table_publisher(
    name: str,
-    col_defs: Union[Dict[str, DType], List[Column]],
+    table_definition: TableDefinitionAlias,


This is a breaking change since the arg is renamed. Should we temporarily support the old parameter with a deprecation warning, or should we directly break code? This only ends up being breaking if the user code is using named parameters.

@jmao-denver

I think I've changed all the relevant param names back.

chipkent · 2024-08-14T16:47:09Z

py/server/tests/test_table_factory.py

        with self.subTest("from table definition"):
-            append_only_input_table = input_table(col_defs=col_defs)
+            append_only_input_table = input_table(col_defs=t.definition)


col_defs is now a misnomer. This is a table definition.

We are inconsistent on if we name the variable as col_defs or table_def across the API.

This item needs some attention.

py/server/deephaven/column.py

py/server/deephaven/table.py

chipkent · 2024-08-14T18:30:25Z

py/server/deephaven/table.py

+    @cached_property
    def is_refreshing(self) -> bool:
        """Whether this table is refreshing."""
-        if self._is_refreshing is None:
-            self._is_refreshing = self.j_table.isRefreshing()
-        return self._is_refreshing
+        return self.j_table.isRefreshing()

-    @property
+    @cached_property
    def is_blink(self) -> bool:
        """Whether this table is a blink table."""
        return _JBlinkTableTools.isBlink(self.j_table)

-    @property
+    @cached_property
    def update_graph(self) -> UpdateGraph:
        """The update graph of the table."""
-        if self._update_graph is None:
-            self._update_graph = UpdateGraph(self.j_table.getUpdateGraph())
-        return self._update_graph
+        return UpdateGraph(self.j_table.getUpdateGraph())

-    @property
+    @cached_property
    def is_flat(self) -> bool:
        """Whether this table is guaranteed to be flat, i.e. its row set will be from 0 to number of rows - 1."""
-        if self._is_flat is None:
-            self._is_flat = self.j_table.isFlat()
-        return self._is_flat
+        return self.j_table.isFlat()


If there is a possibility for mutation, I would remove the caching because we'll never remember to make a change here if we do get to a case where mutation happens. I also doubt that these get called enough to make a significant performance difference.

chipkent · 2024-08-14T18:30:58Z

py/server/deephaven/table.py


-        self._schema = _td_to_columns(self._definition)
-        return self._schema
+    @property


The primary reason to cache is to prevent jpy java calls. In all the cases where cache_property isn't used, it's either all pure python, or the delegated layer is handling the caching.

chipkent · 2024-08-14T18:31:08Z

py/server/deephaven/table.py

+        """The column names of the table."""
+        return list(self.definition.keys())
+
+    @property


py/server/tests/test_table_definition.py

rcaudy

LGTM, assuming you guys get on the same page regarding deprecation/breaking changes, etc.

py/server/deephaven/column.py

rcaudy · 2024-08-14T20:37:11Z

py/server/deephaven/jcompat.py

@@ -327,35 +326,6 @@ def _j_array_to_series(dtype: DType, j_array: jpy.JType, conv_null: bool) -> pd.

    return s

-def j_table_definition(table_definition: Union[Dict[str, DType], List[Column], None]) -> Optional[jpy.JType]:


Was this internal before?

It doesn't have a _, so it wasn't fully private / internal. It is possible someone used it, so it would be a breaking change.

I've added it back with deprecation for removal warning.

rcaudy · 2024-08-14T20:38:21Z

py/server/deephaven/learn/__init__.py

@@ -71,7 +71,7 @@ def _validate(inputs: Input, outputs: Output, table: Table):
    input_columns_list = [input_.input.getColNames()[i] for input_ in inputs for i in
                          range(len(input_.input.getColNames()))]
    input_columns = set(input_columns_list)
-    table_columns = {col.name for col in table.columns}
+    table_columns = set(table.definition.keys())


This is not order-preserving. Why is Python like this? dict is ordered, but set isn't?

The previous set construction was not order preserving either. Looking at the code, the order doesn't matter, it's just after fast check (and I think intersection / difference).

rcaudy · 2024-08-14T20:39:19Z

py/server/deephaven/numpy.py

@@ -95,7 +95,7 @@ def to_numpy(table: Table, cols: List[str] = None) -> np.ndarray:
        if table.is_refreshing:
            table = table.snapshot()

-        col_def_dict = {col.name: col for col in table.columns}
+        col_def_dict = table.definition


Inline and dump the var?

The renamed the variable, but it's a simpler change to not inline... Looking at the method, I think there is room for further improvements, but I don't want to muck with any of this logic atm.

rcaudy · 2024-08-14T20:41:14Z

py/server/deephaven/table.py

+    @cached_property
    def is_refreshing(self) -> bool:
        """Whether this table is refreshing."""
-        if self._is_refreshing is None:
-            self._is_refreshing = self.j_table.isRefreshing()
-        return self._is_refreshing
+        return self.j_table.isRefreshing()

-    @property
+    @cached_property
    def is_blink(self) -> bool:
        """Whether this table is a blink table."""
        return _JBlinkTableTools.isBlink(self.j_table)

-    @property
+    @cached_property
    def update_graph(self) -> UpdateGraph:
        """The update graph of the table."""
-        if self._update_graph is None:
-            self._update_graph = UpdateGraph(self.j_table.getUpdateGraph())
-        return self._update_graph
+        return UpdateGraph(self.j_table.getUpdateGraph())

-    @property
+    @cached_property
    def is_flat(self) -> bool:
        """Whether this table is guaranteed to be flat, i.e. its row set will be from 0 to number of rows - 1."""
-        if self._is_flat is None:
-            self._is_flat = self.j_table.isFlat()
-        return self._is_flat
+        return self.j_table.isFlat()


I'm convinced the caching is fine. But I also agree that we are likely not calling these anywhere performance critical.

chipkent · 2024-08-15T04:46:37Z

py/server/deephaven/column.py

-    input_data: Any = field(default=None)
-
-    def __post_init__(self):
+    def j_object(self) -> jpy.JType:


I don't think so, but do a quick check.

chipkent · 2024-08-15T04:49:39Z

py/server/deephaven/column.py

+        return ColumnType(self.j_column_definition.getColumnType())
+
+
+class Column(ColumnDefinition):


I'm ok with deprecation with a plan to remove in the next release. Create a ticket for removal and assign it to one of us.

py/server/deephaven/column.py

chipkent · 2024-08-15T05:02:59Z

py/server/deephaven/jcompat.py

@@ -327,35 +326,6 @@ def _j_array_to_series(dtype: DType, j_array: jpy.JType, conv_null: bool) -> pd.

    return s

-def j_table_definition(table_definition: Union[Dict[str, DType], List[Column], None]) -> Optional[jpy.JType]:


It doesn't have a _, so it wasn't fully private / internal. It is possible someone used it, so it would be a breaking change.

chipkent · 2024-08-15T05:08:50Z

py/server/tests/test_table_factory.py

        with self.subTest("from table definition"):
-            append_only_input_table = input_table(col_defs=col_defs)
+            append_only_input_table = input_table(col_defs=t.definition)


This item needs some attention.

…tion

deephaven-internal · 2024-08-15T23:04:56Z

Labels indicate documentation is required. Issues for documentation have been opened:

Community: deephaven/deephaven-docs-community#295

feat: Add TableDefinition wrapper for python

8105b15

devinrsmith added DocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Aug 3, 2024

devinrsmith self-assigned this Aug 3, 2024

reuse name

7ae04cd

This was referenced Aug 3, 2024

feat: provide TableDefinition functions for Iceberg tables #5891

Merged

Method for getting a specific column's info in Python #5655

Open

devinrsmith added 2 commits August 5, 2024 06:48

Merge remote-tracking branch 'upstream/main' into python-table-defini…

c08506c

…tion

erge remote-tracking branch 'upstream/main' into python-table-definition

a57fe4e

chipkent reviewed Aug 9, 2024

View reviewed changes

py/server/deephaven/table.py Outdated Show resolved Hide resolved

devinrsmith added 4 commits August 12, 2024 09:06

Update Column as wrapper of ColumnDefinition. Update InputColumn. Add…

4c08531

… cached_property.

Merge remote-tracking branch 'upstream/main' into nightly/python-tabl…

8d18f68

…e-definition

fix List[Column]

3ed4b41

Fix some tests, cache name

3780ab4

devinrsmith marked this pull request as ready for review August 13, 2024 14:01

devinrsmith requested review from jmao-denver and rcaudy as code owners August 13, 2024 14:01

devinrsmith requested a review from chipkent August 13, 2024 14:03

rcaudy requested changes Aug 13, 2024

View reviewed changes

jmao-denver reviewed Aug 13, 2024

View reviewed changes

Add tests

8dbe38c

devinrsmith requested review from jmao-denver and rcaudy August 13, 2024 15:43

jmao-denver reviewed Aug 13, 2024

View reviewed changes

py/server/deephaven/column.py Outdated Show resolved Hide resolved

Review response

d1fca92

devinrsmith requested a review from jmao-denver August 13, 2024 17:54

Update callers

52a404a

devinrsmith requested review from rcaudy and jmao-denver August 13, 2024 22:23

chipkent reviewed Aug 14, 2024

View reviewed changes

devinrsmith added 2 commits August 14, 2024 10:42

Remove TableDefinition.of

0053787

Pydoc review responses

e42c7ae

chipkent reviewed Aug 14, 2024

View reviewed changes

Update naming to TableDefinitionLike

84c6de7

rcaudy reviewed Aug 14, 2024

View reviewed changes

Review comments

81229ea

devinrsmith requested a review from chipkent August 15, 2024 01:15

chipkent reviewed Aug 15, 2024

View reviewed changes

devinrsmith mentioned this pull request Aug 15, 2024

Remove deprecated python items related to TableDefinition #5940

Open

Review response

13b3cf0

devinrsmith requested review from rcaudy and chipkent August 15, 2024 14:31

devinrsmith changed the title ~~feat: Add TableDefinition wrapper for python~~ feat!: Add TableDefinition wrapper for python Aug 15, 2024

chipkent previously approved these changes Aug 15, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into python-table-defini…

835c2fc

…tion

devinrsmith dismissed chipkent’s stale review via 835c2fc August 15, 2024 21:23

rcaudy previously approved these changes Aug 15, 2024

View reviewed changes

Deprecate json_spec col_defs List[Tuple[str, DType]]

0b9103c

devinrsmith dismissed rcaudy’s stale review via 0b9103c August 15, 2024 22:36

jmao-denver approved these changes Aug 15, 2024

View reviewed changes

devinrsmith enabled auto-merge (squash) August 15, 2024 22:44

rcaudy approved these changes Aug 15, 2024

View reviewed changes

devinrsmith merged commit 07786ef into deephaven:main Aug 15, 2024
16 checks passed

devinrsmith deleted the python-table-definition branch August 15, 2024 23:04

github-actions bot locked and limited conversation to collaborators Aug 15, 2024

		@@ -327,35 +326,6 @@ def _j_array_to_series(dtype: DType, j_array: jpy.JType, conv_null: bool) -> pd.

		return s

		def j_table_definition(table_definition: Union[Dict[str, DType], List[Column], None]) -> Optional[jpy.JType]:

		return ColumnType(self.j_column_definition.getColumnType())


		class Column(ColumnDefinition):

feat!: Add TableDefinition wrapper for python #5892

feat!: Add TableDefinition wrapper for python #5892

Conversation

devinrsmith commented Aug 3, 2024 • edited Loading

devinrsmith commented Aug 3, 2024

jmao-denver commented Aug 6, 2024

devinrsmith commented Aug 6, 2024

chipkent left a comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmao-denver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deephaven-internal commented Aug 15, 2024

devinrsmith commented Aug 3, 2024 •

edited

Loading

devinrsmith Aug 15, 2024 •

edited

Loading