From 0f386a996e5ea2935bf019fea960977d8b21d7ac Mon Sep 17 00:00:00 2001 From: Arman Farhangi Date: Fri, 15 Mar 2024 23:14:25 -0700 Subject: [PATCH 01/11] Fix input table jpy restriction --- py/server/deephaven/table_factory.py | 1 - py/server/tests/test_table_factory.py | 8 +++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/py/server/deephaven/table_factory.py b/py/server/deephaven/table_factory.py index c9689b84c77..c610426b43b 100644 --- a/py/server/deephaven/table_factory.py +++ b/py/server/deephaven/table_factory.py @@ -235,7 +235,6 @@ class InputTable(Table): Users should always create InputTables through factory methods rather than directly from the constructor. """ - j_object_type = _JBaseArrayBackedInputTable def __init__(self, j_table: jpy.JType): super().__init__(j_table) diff --git a/py/server/tests/test_table_factory.py b/py/server/tests/test_table_factory.py index 0e4aa080ad1..e5bf0adb17e 100644 --- a/py/server/tests/test_table_factory.py +++ b/py/server/tests/test_table_factory.py @@ -13,7 +13,7 @@ from deephaven.column import byte_col, char_col, short_col, bool_col, int_col, long_col, float_col, double_col, \ string_col, datetime_col, pyobj_col, jobj_col from deephaven.constants import NULL_DOUBLE, NULL_FLOAT, NULL_LONG, NULL_INT, NULL_SHORT, NULL_BYTE -from deephaven.table_factory import DynamicTableWriter, ring_table +from deephaven.table_factory import DynamicTableWriter, InputTable, ring_table from tests.testbase import BaseTestCase from deephaven.table import Table from deephaven.stream import blink_to_append_only, stream_to_append_only @@ -21,6 +21,7 @@ JArrayList = jpy.get_type("java.util.ArrayList") _JBlinkTableTools = jpy.get_type("io.deephaven.engine.table.impl.BlinkTableTools") _JDateTimeUtils = jpy.get_type("io.deephaven.time.DateTimeUtils") +_JTable = jpy.get_type("io.deephaven.engine.table.Table") @dataclass @@ -372,6 +373,11 @@ def test_input_table(self): keyed_input_table.delete(t.select(["String", "Double"])) self.assertEqual(keyed_input_table.size, 0) + with self.subTest("custom input table creation"): + place_holder_input_table = empty_table(1).update_view(["Key=`A`", "Value=10"]).with_attributes({_JTable.INPUT_TABLE_ATTRIBUTE: "Placeholder IT"}).j_table + # Confirming no error. + InputTable(place_holder_input_table) + def test_ring_table(self): cols = [ bool_col(name="Boolean", data=[True, False]), From e44039f4ab251f5576dfc09cd0ac3a622c07ac0f Mon Sep 17 00:00:00 2001 From: Arman Farhangi Date: Fri, 15 Mar 2024 23:27:55 -0700 Subject: [PATCH 02/11] Remove unnecessary jpy type --- py/server/deephaven/table_factory.py | 1 - 1 file changed, 1 deletion(-) diff --git a/py/server/deephaven/table_factory.py b/py/server/deephaven/table_factory.py index c610426b43b..445cf88874c 100644 --- a/py/server/deephaven/table_factory.py +++ b/py/server/deephaven/table_factory.py @@ -23,7 +23,6 @@ _JTableFactory = jpy.get_type("io.deephaven.engine.table.TableFactory") _JTableTools = jpy.get_type("io.deephaven.engine.util.TableTools") _JDynamicTableWriter = jpy.get_type("io.deephaven.engine.table.impl.util.DynamicTableWriter") -_JBaseArrayBackedInputTable = jpy.get_type("io.deephaven.engine.table.impl.util.BaseArrayBackedInputTable") _JAppendOnlyArrayBackedInputTable = jpy.get_type( "io.deephaven.engine.table.impl.util.AppendOnlyArrayBackedInputTable") _JKeyedArrayBackedInputTable = jpy.get_type("io.deephaven.engine.table.impl.util.KeyedArrayBackedInputTable") From cb2792d85fbd05c7c061830d5d82ddb1c57de89d Mon Sep 17 00:00:00 2001 From: jianfengmao Date: Tue, 19 Mar 2024 21:39:26 -0600 Subject: [PATCH 03/11] Wrap java object with the most specific wrapper --- py/server/deephaven/_wrapper.py | 36 +++++++++++++++++++++------ py/server/tests/test_table_factory.py | 14 +++++++++++ 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/py/server/deephaven/_wrapper.py b/py/server/deephaven/_wrapper.py index cc11e7e835f..df7d6edf88c 100644 --- a/py/server/deephaven/_wrapper.py +++ b/py/server/deephaven/_wrapper.py @@ -13,7 +13,7 @@ import pkgutil import sys from abc import ABC, abstractmethod -from typing import Set, Union, Optional, Any +from typing import Set, Union, Optional, Any, List import jpy @@ -102,7 +102,7 @@ def _is_direct_initialisable(cls) -> bool: return False -def _lookup_wrapped_class(j_obj: jpy.JType) -> Optional[type]: +def _lookup_wrapped_class(j_obj: jpy.JType) -> List[type]: """ Returns the wrapper class for the specified Java object. """ # load every module in the deephaven package so that all the wrapper classes are loaded and available to wrap # the Java objects returned by calling resolve() @@ -111,12 +111,13 @@ def _lookup_wrapped_class(j_obj: jpy.JType) -> Optional[type]: _recursive_import(__package__.partition(".")[0]) _has_all_wrappers_imported = True + wcs = [] for wc in _di_wrapper_classes: j_clz = wc.j_object_type if j_clz.jclass.isInstance(j_obj): - return wc + wcs.append(wc) - return None + return wcs def javaify(obj: Any) -> Optional[jpy.JType]: @@ -162,15 +163,36 @@ def pythonify(j_obj: Any) -> Optional[Any]: return wrap_j_object(j_obj) +def _wrap_with_subclass(j_obj: jpy.JType, cls: type) -> Optional[JObjectWrapper]: + """ Returns a wrapper instance for the specified Java object if the specified class is a subclass of the wrapper + class. Otherwise, returns None. """ + subclasses = cls.__subclasses__() + for subclass in subclasses: + try: + if (wrapper := _wrap_with_subclass(j_obj, subclass)) is not None: + return wrapper + return subclass(j_obj) + except: + continue + return None + def wrap_j_object(j_obj: jpy.JType) -> Union[JObjectWrapper, jpy.JType]: - """ Wraps the specified Java object as an instance of a custom wrapper class if one is available, otherwise returns + """ Wraps the specified Java object as an instance of the most specific custom wrapper class if one is available, + otherwise returns the raw Java object. """ if j_obj is None: return None - wc = _lookup_wrapped_class(j_obj) + wcs = _lookup_wrapped_class(j_obj) + for wc in wcs: + try: + if (wrapper:= _wrap_with_subclass(j_obj, wc)) is not None: + return wrapper + return wc(j_obj) + except: + continue - return wc(j_obj) if wc else j_obj + return j_obj def unwrap(obj: Any) -> Union[jpy.JType, Any]: diff --git a/py/server/tests/test_table_factory.py b/py/server/tests/test_table_factory.py index e5bf0adb17e..266364cf61f 100644 --- a/py/server/tests/test_table_factory.py +++ b/py/server/tests/test_table_factory.py @@ -456,5 +456,19 @@ def test_input_table_empty_data(self): it.delete(t) self.assertEqual(it.size, 0) + def test_j_input_wrapping(self): + cols = [ + bool_col(name="Boolean", data=[True, False]), + string_col(name="String", data=["foo", "bar"]), + ] + t = new_table(cols=cols) + col_defs = {c.name: c.data_type for c in t.columns} + append_only_input_table = input_table(col_defs=col_defs) + + from deephaven import _wrapper + t = _wrapper.wrap_j_object(append_only_input_table.j_table) + self.assertTrue(isinstance(t, InputTable)) + + if __name__ == '__main__': unittest.main() From 6e9e9e3e3eedd3f8cd4a210046561a70f6563c90 Mon Sep 17 00:00:00 2001 From: jianfengmao Date: Tue, 19 Mar 2024 21:54:42 -0600 Subject: [PATCH 04/11] Fix docstring format issue --- py/server/deephaven/_wrapper.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/py/server/deephaven/_wrapper.py b/py/server/deephaven/_wrapper.py index df7d6edf88c..71c6a0e253f 100644 --- a/py/server/deephaven/_wrapper.py +++ b/py/server/deephaven/_wrapper.py @@ -178,8 +178,7 @@ def _wrap_with_subclass(j_obj: jpy.JType, cls: type) -> Optional[JObjectWrapper] def wrap_j_object(j_obj: jpy.JType) -> Union[JObjectWrapper, jpy.JType]: """ Wraps the specified Java object as an instance of the most specific custom wrapper class if one is available, - otherwise returns - the raw Java object. """ + otherwise returns the raw Java object. """ if j_obj is None: return None From 0a1cc2093989151d2ab8b61cb7af2457a3c3f88d Mon Sep 17 00:00:00 2001 From: Jianfeng Mao <4297243+jmao-denver@users.noreply.github.com> Date: Wed, 20 Mar 2024 09:19:56 -0600 Subject: [PATCH 05/11] Apply suggestions from code review Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com> --- py/server/deephaven/_wrapper.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/py/server/deephaven/_wrapper.py b/py/server/deephaven/_wrapper.py index 71c6a0e253f..6c8be5756a2 100644 --- a/py/server/deephaven/_wrapper.py +++ b/py/server/deephaven/_wrapper.py @@ -103,7 +103,7 @@ def _is_direct_initialisable(cls) -> bool: def _lookup_wrapped_class(j_obj: jpy.JType) -> List[type]: - """ Returns the wrapper class for the specified Java object. """ + """ Returns the wrapper classes for the specified Java object. """ # load every module in the deephaven package so that all the wrapper classes are loaded and available to wrap # the Java objects returned by calling resolve() global _has_all_wrappers_imported @@ -166,8 +166,7 @@ def pythonify(j_obj: Any) -> Optional[Any]: def _wrap_with_subclass(j_obj: jpy.JType, cls: type) -> Optional[JObjectWrapper]: """ Returns a wrapper instance for the specified Java object if the specified class is a subclass of the wrapper class. Otherwise, returns None. """ - subclasses = cls.__subclasses__() - for subclass in subclasses: + for subclass in cls.__subclasses__(): try: if (wrapper := _wrap_with_subclass(j_obj, subclass)) is not None: return wrapper From 42765af9ef97802d61a8cf7fe7f68298a23cf931 Mon Sep 17 00:00:00 2001 From: jianfengmao Date: Wed, 20 Mar 2024 09:56:25 -0600 Subject: [PATCH 06/11] Respond to review comments --- py/server/deephaven/_wrapper.py | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/py/server/deephaven/_wrapper.py b/py/server/deephaven/_wrapper.py index 6c8be5756a2..4d7efd56758 100644 --- a/py/server/deephaven/_wrapper.py +++ b/py/server/deephaven/_wrapper.py @@ -102,7 +102,7 @@ def _is_direct_initialisable(cls) -> bool: return False -def _lookup_wrapped_class(j_obj: jpy.JType) -> List[type]: +def _lookup_wrapped_class(j_obj: jpy.JType) -> List[JObjectWrapper]: """ Returns the wrapper classes for the specified Java object. """ # load every module in the deephaven package so that all the wrapper classes are loaded and available to wrap # the Java objects returned by calling resolve() @@ -111,13 +111,7 @@ def _lookup_wrapped_class(j_obj: jpy.JType) -> List[type]: _recursive_import(__package__.partition(".")[0]) _has_all_wrappers_imported = True - wcs = [] - for wc in _di_wrapper_classes: - j_clz = wc.j_object_type - if j_clz.jclass.isInstance(j_obj): - wcs.append(wc) - - return wcs + return [wc for wc in _di_wrapper_classes if wc.j_object_type.jclass.isInstance(j_obj)] def javaify(obj: Any) -> Optional[jpy.JType]: @@ -164,8 +158,16 @@ def pythonify(j_obj: Any) -> Optional[Any]: def _wrap_with_subclass(j_obj: jpy.JType, cls: type) -> Optional[JObjectWrapper]: - """ Returns a wrapper instance for the specified Java object if the specified class is a subclass of the wrapper - class. Otherwise, returns None. """ + """ Returns a wrapper instance for the specified Java object by trying the entire subclasses' hierarchy. The + function employs the DFS strategy to try most specific subclass first. If no matching wrapper class is found, + returns None. + + The premises for this function are as follows: + - The subclasses all share the same class attribute `j_object_type` (guaranteed by subclassing JObjectWrapper) + - The subclasses are all direct initialisable (guaranteed by subclassing JObjectWrapper) + - The subclasses are all distinct from each other and check for their uniqueness in the initializer (__init__), e.g. + InputTable checks for the presence of the INPUT_TABLE_ATTRIBUTE attribute on the Java object. + """ for subclass in cls.__subclasses__(): try: if (wrapper := _wrap_with_subclass(j_obj, subclass)) is not None: @@ -175,7 +177,8 @@ def _wrap_with_subclass(j_obj: jpy.JType, cls: type) -> Optional[JObjectWrapper] continue return None -def wrap_j_object(j_obj: jpy.JType) -> Union[JObjectWrapper, jpy.JType]: + +def wrap_j_object(j_obj: jpy.JType) -> Optional[Union[JObjectWrapper, jpy.JType]]: """ Wraps the specified Java object as an instance of the most specific custom wrapper class if one is available, otherwise returns the raw Java object. """ if j_obj is None: From 302321f3e4ebd7f0ec79e18c2180e21ce3f53d0e Mon Sep 17 00:00:00 2001 From: jianfengmao Date: Wed, 20 Mar 2024 10:03:25 -0600 Subject: [PATCH 07/11] Tiny coding formatting issue --- py/server/deephaven/_wrapper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/py/server/deephaven/_wrapper.py b/py/server/deephaven/_wrapper.py index 4d7efd56758..338f2f6884e 100644 --- a/py/server/deephaven/_wrapper.py +++ b/py/server/deephaven/_wrapper.py @@ -193,7 +193,7 @@ def wrap_j_object(j_obj: jpy.JType) -> Optional[Union[JObjectWrapper, jpy.JType] except: continue - return j_obj + return j_obj def unwrap(obj: Any) -> Union[jpy.JType, Any]: From b143ab712ec14217fa359e3a508e8f630cb10c39 Mon Sep 17 00:00:00 2001 From: Jianfeng Mao <4297243+jmao-denver@users.noreply.github.com> Date: Wed, 20 Mar 2024 14:08:00 -0600 Subject: [PATCH 08/11] Update py/server/deephaven/_wrapper.py Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com> --- py/server/deephaven/_wrapper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/py/server/deephaven/_wrapper.py b/py/server/deephaven/_wrapper.py index 338f2f6884e..59df56be97c 100644 --- a/py/server/deephaven/_wrapper.py +++ b/py/server/deephaven/_wrapper.py @@ -159,7 +159,7 @@ def pythonify(j_obj: Any) -> Optional[Any]: def _wrap_with_subclass(j_obj: jpy.JType, cls: type) -> Optional[JObjectWrapper]: """ Returns a wrapper instance for the specified Java object by trying the entire subclasses' hierarchy. The - function employs the DFS strategy to try most specific subclass first. If no matching wrapper class is found, + function employs a Depth First Search strategy to try the most specific subclass first. If no matching wrapper class is found, returns None. The premises for this function are as follows: From 672a2edb534c7d49ecbdcd26ab25214e44611d9e Mon Sep 17 00:00:00 2001 From: jianfengmao Date: Wed, 20 Mar 2024 14:19:14 -0600 Subject: [PATCH 09/11] Add one more test case --- py/server/tests/test_table_factory.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/py/server/tests/test_table_factory.py b/py/server/tests/test_table_factory.py index 266364cf61f..8a526ceddc1 100644 --- a/py/server/tests/test_table_factory.py +++ b/py/server/tests/test_table_factory.py @@ -9,7 +9,7 @@ import numpy as np from deephaven import DHError, read_csv, time_table, empty_table, merge, merge_sorted, dtypes, new_table, \ - input_table, time + input_table, time, _wrapper from deephaven.column import byte_col, char_col, short_col, bool_col, int_col, long_col, float_col, double_col, \ string_col, datetime_col, pyobj_col, jobj_col from deephaven.constants import NULL_DOUBLE, NULL_FLOAT, NULL_LONG, NULL_INT, NULL_SHORT, NULL_BYTE @@ -376,7 +376,10 @@ def test_input_table(self): with self.subTest("custom input table creation"): place_holder_input_table = empty_table(1).update_view(["Key=`A`", "Value=10"]).with_attributes({_JTable.INPUT_TABLE_ATTRIBUTE: "Placeholder IT"}).j_table # Confirming no error. - InputTable(place_holder_input_table) + it = InputTable(place_holder_input_table) + + self.assertTrue(isinstance(_wrapper.wrap_j_object(place_holder_input_table), InputTable)) + def test_ring_table(self): cols = [ @@ -465,7 +468,6 @@ def test_j_input_wrapping(self): col_defs = {c.name: c.data_type for c in t.columns} append_only_input_table = input_table(col_defs=col_defs) - from deephaven import _wrapper t = _wrapper.wrap_j_object(append_only_input_table.j_table) self.assertTrue(isinstance(t, InputTable)) From 319a68d6de9cd8bbb0d16b5d2e8fd0116c07115d Mon Sep 17 00:00:00 2001 From: jianfengmao Date: Wed, 20 Mar 2024 17:59:49 -0600 Subject: [PATCH 10/11] Empty Commit to force rerun of CI From 47e866cfc6be899ee690d1ea691eef9ac9d1bedf Mon Sep 17 00:00:00 2001 From: jianfengmao Date: Wed, 20 Mar 2024 21:56:46 -0600 Subject: [PATCH 11/11] Respond to latest review comments --- py/server/deephaven/table_factory.py | 3 +++ py/server/tests/test_table_factory.py | 16 +++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/py/server/deephaven/table_factory.py b/py/server/deephaven/table_factory.py index 445cf88874c..7efff4b534c 100644 --- a/py/server/deephaven/table_factory.py +++ b/py/server/deephaven/table_factory.py @@ -29,6 +29,7 @@ _JTableDefinition = jpy.get_type("io.deephaven.engine.table.TableDefinition") _JTable = jpy.get_type("io.deephaven.engine.table.Table") _J_INPUT_TABLE_ATTRIBUTE = _JTable.INPUT_TABLE_ATTRIBUTE +_J_InputTableUpdater = jpy.get_type("io.deephaven.engine.util.input.InputTableUpdater") _JRingTableTools = jpy.get_type("io.deephaven.engine.table.impl.sources.ring.RingTableTools") _JSupplier = jpy.get_type('java.util.function.Supplier') _JFunctionGeneratedTableFactory = jpy.get_type("io.deephaven.engine.table.impl.util.FunctionGeneratedTableFactory") @@ -240,6 +241,8 @@ def __init__(self, j_table: jpy.JType): self.j_input_table = self.j_table.getAttribute(_J_INPUT_TABLE_ATTRIBUTE) if not self.j_input_table: raise DHError("the provided table input is not suitable for input tables.") + if not _J_InputTableUpdater.jclass.isInstance(self.j_input_table): + raise DHError("the provided table's InputTable attribute type is not of InputTableUpdater type.") def add(self, table: Table) -> None: """Synchronously writes rows from the provided table to this input table. If this is a keyed input table, added rows with keys diff --git a/py/server/tests/test_table_factory.py b/py/server/tests/test_table_factory.py index 8a526ceddc1..3b1cfe55062 100644 --- a/py/server/tests/test_table_factory.py +++ b/py/server/tests/test_table_factory.py @@ -375,10 +375,12 @@ def test_input_table(self): with self.subTest("custom input table creation"): place_holder_input_table = empty_table(1).update_view(["Key=`A`", "Value=10"]).with_attributes({_JTable.INPUT_TABLE_ATTRIBUTE: "Placeholder IT"}).j_table - # Confirming no error. - it = InputTable(place_holder_input_table) - self.assertTrue(isinstance(_wrapper.wrap_j_object(place_holder_input_table), InputTable)) + with self.assertRaises(DHError) as cm: + InputTable(place_holder_input_table) + self.assertIn("not of InputTableUpdater type", str(cm.exception)) + + self.assertTrue(isinstance(_wrapper.wrap_j_object(place_holder_input_table), Table)) def test_ring_table(self): @@ -468,8 +470,12 @@ def test_j_input_wrapping(self): col_defs = {c.name: c.data_type for c in t.columns} append_only_input_table = input_table(col_defs=col_defs) - t = _wrapper.wrap_j_object(append_only_input_table.j_table) - self.assertTrue(isinstance(t, InputTable)) + it = _wrapper.wrap_j_object(append_only_input_table.j_table) + self.assertTrue(isinstance(it, InputTable)) + + t = _wrapper.wrap_j_object(t.j_object) + self.assertFalse(isinstance(t, InputTable)) + self.assertTrue(isinstance(t, Table)) if __name__ == '__main__':