fix: DH-20658: Complete wrapping of the structured Filters in Core Py API #7482

jmao-denver · 2025-12-09T16:29:56Z

Note:

no wrapping of the cyclic referencing Expression class or the Literal class, in favor of the native Python types
no wrapping of the Function and Method classes; workaround exists in the form of the class method from_ that takes in a raw string and returns a Filter object

github-actions · 2025-12-09T16:30:05Z

No docs changes detected for `f28bd08`

cpwright

I think leaving literal out of the API makes perfect sense. I do think that we should think about more general expressions, so we can interact with QST-like things.

cpwright · 2025-12-11T14:46:58Z

py/server/deephaven/filters.py

        raise DHError(e, "failed to create a pattern filter.") from e
+
+
+def in_(col: str, values: Sequence[Union[bool, int, float, str]]) -> Filter:


I wonder if this should be inverted, i.e. a union of the different sequence types? It seems mixing types might be odd.

deephaven-core/engine/table/src/test/java/io/deephaven/engine/table/impl/select/WhereFilterTest.java

Line 298 in 01da8df

public void testInLiteralsDifferentTypes() {

I was influenced by this test in Java, and that we allow columns of the generic Object type. The fact that we are not as limiting as the inverted version is also a positive.

py/server/deephaven/filters.py

py/server/tests/test_filters.py

Copilot

Pull request overview

This PR completes the wrapping of structured filters in the Core Python API by adding support for comparison operators (eq, ne, lt, le, gt, ge) and the in_ filter. A new ColumnName string subclass is introduced to distinguish column references from literal values in filter expressions. The implementation delegates to Java filter APIs while avoiding wrapping of cyclic Expression and Literal classes in favor of native Python types.

Added comparison filter functions (eq, ne, lt, le, gt, ge) with support for both literals and column names
Added in_ filter function for membership testing
Introduced ColumnName class to differentiate column references from string literals
Updated not_ function to use static Java method instead of instance method

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
py/server/deephaven/filters.py	Adds comparison and `in_` filter functions, introduces `ColumnName` class, fixes grammar in module docstring, and updates `not_` implementation
py/server/tests/test_filters.py	Adds comprehensive test coverage for new `in_` and comparison filter functions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

py/server/tests/test_filters.py

py/server/deephaven/filters.py

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

py/server/deephaven/filters.py

py/server/tests/test_filters.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-19T20:03:30Z

py/server/deephaven/filters.py

 from deephaven.jcompat import _to_sequence

 _JFilter = jpy.get_type("io.deephaven.api.filter.Filter")
+_JFilterNot = jpy.get_type("io.deephaven.api.filter.FilterNot")


The import JFilterNot is unused. It was likely added as part of the refactoring but is not actually used in the implementation since the not() function and Filter.not_() method both use getattr(_JFilter, "not") instead.

Suggested change

_JFilterNot = jpy.get_type("io.deephaven.api.filter.FilterNot")

Copilot · 2025-12-19T20:03:30Z

py/server/deephaven/filters.py

+def eq(
+    left: Union[bool, int, float, str, ColumnName],
+    right: Union[bool, int, float, str, ColumnName],
+) -> Filter:
+    """Creates a new filter that evaluates to true when the left operand is equal to the right operand.
+
+    Args:
+        left (Union[bool, int, float, str, ColumnName]): the left operand, either a literal value or a column name
+        right (Union[bool, int, float, str, ColumnName]): the right operand, either a literal value or a column name
+
+    Returns:
+        Filter: a new equality filter
+    """
+    return Filter(j_filter=_j_filter_comparison("eq", left=left, right=right))
+
+
+def ne(
+    left: Union[bool, int, float, str, ColumnName],
+    right: Union[bool, int, float, str, ColumnName],
+) -> Filter:
+    """Creates a new filter that evaluates to true when the left operand is not equal to the right operand.
+
+    Args:
+        left (Union[bool, int, float, str, ColumnName]): the left operand, either a literal value or a column name
+        right (Union[bool, int, float, str, ColumnName]): the right operand, either a literal value or a column name
+
+    Returns:
+        Filter: a new inequality filter
+    """
+    return Filter(j_filter=_j_filter_comparison("ne", left=left, right=right))
+
+
+def lt(
+    left: Union[bool, int, float, str, ColumnName],
+    right: Union[bool, int, float, str, ColumnName],
+) -> Filter:
+    """Creates a new filter that evaluates to true when the left operand is less than the right operand.
+
+    Args:
+        left (Union[bool, int, float, str, ColumnName]): the left operand, either a literal value or a column name
+        right (Union[bool, int, float, str, ColumnName]): the right operand, either a literal value or a column name
+
+    Returns:
+        Filter: a new less-than filter
+    """
+    return Filter(j_filter=_j_filter_comparison("lt", left=left, right=right))
+
+
+def le(
+    left: Union[bool, int, float, str, ColumnName],
+    right: Union[bool, int, float, str, ColumnName],
+) -> Filter:
+    """Creates a new filter that evaluates to true when the left operand is less than or equal to the right operand.
+
+    Args:
+        left (Union[bool, int, float, str, ColumnName]): the left operand, either a literal value or a column name
+        right (Union[bool, int, float, str, ColumnName]): the right operand, either a literal value or a column name
+
+    Returns:
+        Filter: a new less-than-or-equal filter
+    """
+    return Filter(j_filter=_j_filter_comparison("le", left=left, right=right))
+
+
+def gt(
+    left: Union[bool, int, float, str, ColumnName],
+    right: Union[bool, int, float, str, ColumnName],
+) -> Filter:
+    """Creates a new filter that evaluates to true when the left operand is greater than the right operand.
+
+    Args:
+        left (Union[bool, int, float, str, ColumnName]): the left operand, either a literal value or a column name
+        right (Union[bool, int, float, str, ColumnName]): the right operand, either a literal value or a column name
+
+    Returns:
+        Filter: a new greater-than filter
+    """
+    return Filter(j_filter=_j_filter_comparison("gt", left=left, right=right))
+
+
+def ge(
+    left: Union[bool, int, float, str, ColumnName],
+    right: Union[bool, int, float, str, ColumnName],
+) -> Filter:
+    """Creates a new filter that evaluates to true when the left operand is greater than or equal to the right operand.
+
+    Args:
+        left (Union[bool, int, float, str, ColumnName]): the left operand, either a literal value or a column name
+        right (Union[bool, int, float, str, ColumnName]): the right operand, either a literal value or a column name
+
+    Returns:
+        Filter: a new greater-than-or-equal filter
+    """
+    return Filter(j_filter=_j_filter_comparison("ge", left=left, right=right))


The comparison filter functions (eq, ne, lt, le, gt, ge) lack error handling. Unlike other filter creation functions in this module (in_, pattern, is_null, etc.), these functions don't wrap their Java calls in try-except blocks to raise DHError on failure. This is inconsistent with the established error handling pattern in this module.

Copilot · 2025-12-19T20:03:31Z

py/server/deephaven/filters.py

+        raise DHError(e, "failed to create an in filter.") from e
+
+
+_FILTER_COMPARISON_MAP: dict = {


The type annotation for _FILTER_COMPARISON_MAP is incomplete. It should specify the key and value types, e.g., dict[str, Callable] or more specifically dict[str, Any] if the exact Java method type cannot be expressed in Python typing.

Copilot · 2025-12-19T20:03:31Z

py/server/tests/test_filters.py

+                t.where(filter_in)
+            print(cm.exception)
+
+            # inconsistent behavior observed, should fail with https://deephaven.atlassian.net/browse/DH-21232 fixed


The comment mentions there's a ticket DH-21232 that should fix inconsistent behavior, but the test code doesn't clearly demonstrate what the expected behavior should be once the ticket is fixed. Consider adding a TODO comment or skipping this test case until the inconsistency is resolved, or at least document what the expected behavior should be after the fix.

Suggested change

# inconsistent behavior observed, should fail with https://deephaven.atlassian.net/browse/DH-21232 fixed

# TODO(DH-21232): Once DH-21232 is fixed, mixed-type values in an `in_` filter on a string

# column (e.g., in_("B", [2, "3"])) should behave consistently with the above case for column

# "A" and raise a DHError instead of returning a filtered table. At that point, update this

# subtest to expect a DHError (similar to the `with self.assertRaises(DHError)` block above)

# and remove or adjust the size assertion below.

Complete wrapping of structured Filters

4c0ef17

jmao-denver self-assigned this Dec 9, 2025

jmao-denver added NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels Dec 9, 2025

jmao-denver marked this pull request as ready for review December 10, 2025 18:51

jmao-denver requested review from chipkent, cpwright and rcaudy as code owners December 10, 2025 18:51

cpwright reviewed Dec 11, 2025

View reviewed changes

jmao-denver requested a review from Copilot December 16, 2025 17:53

Copilot AI reviewed Dec 16, 2025

View reviewed changes

py/server/tests/test_filters.py Show resolved Hide resolved

py/server/tests/test_filters.py Outdated Show resolved Hide resolved

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

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

Respond to review comments

1c2b822

jmao-denver requested review from Copilot and cpwright December 19, 2025 19:06

Copilot started reviewing on behalf of jmao-denver December 19, 2025 19:06 View session

Copilot AI reviewed Dec 19, 2025

View reviewed changes

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

py/server/tests/test_filters.py Outdated Show resolved Hide resolved

jmao-denver and others added 2 commits December 19, 2025 13:00

Update py/server/tests/test_filters.py

80eecf2

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update py/server/deephaven/filters.py

f28bd08

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

jmao-denver requested a review from Copilot December 19, 2025 20:00

Copilot started reviewing on behalf of jmao-denver December 19, 2025 20:01 View session

Copilot AI reviewed Dec 19, 2025

View reviewed changes

		raise DHError(e, "failed to create a pattern filter.") from e


		def in_(col: str, values: Sequence[Union[bool, int, float, str]]) -> Filter:

		raise DHError(e, "failed to create an in filter.") from e


		_FILTER_COMPARISON_MAP: dict = {

-            # inconsistent behavior observed, should fail with https://deephaven.atlassian.net/browse/DH-21232 fixed
+            # TODO(DH-21232): Once DH-21232 is fixed, mixed-type values in an `in_` filter on a string
+            # column (e.g., in_("B", [2, "3"])) should behave consistently with the above case for column
+            # "A" and raise a DHError instead of returning a filtered table. At that point, update this
+            # subtest to expect a DHError (similar to the `with self.assertRaises(DHError)` block above)
+            # and remove or adjust the size assertion below.

fix: DH-20658: Complete wrapping of the structured Filters in Core Py API #7482

Are you sure you want to change the base?

fix: DH-20658: Complete wrapping of the structured Filters in Core Py API #7482

Uh oh!

Conversation

jmao-denver commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No docs changes detected for f28bd08

Uh oh!

cpwright left a comment

Choose a reason for hiding this comment

Uh oh!

cpwright Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

jmao-denver Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jmao-denver commented Dec 9, 2025 •

edited

Loading

github-actions bot commented Dec 9, 2025 •

edited

Loading

No docs changes detected for `f28bd08`