fix: DH-20658: Complete wrapping of the structured Filters in Core Py API #7482
Conversation
jmao-denver
added
NoDocumentationNeeded
NoReleaseNotesNeeded
No docs changes detected for c629e4f |
jmao-denver
requested review from
chipkent,
cpwright and
rcaudy
as code owners
cpwright
left a comment
cpwright
left a comment
There was a problem hiding this comment.
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.
| raise DHError(e, "failed to create a pattern filter.") from e | ||
|
|
||
|
|
||
| def in_(col: str, values: Sequence[Union[bool, int, float, str]]) -> Filter: |
There was a problem hiding this comment.
I wonder if this should be inverted, i.e. a union of the different sequence types? It seems mixing types might be odd.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
ColumnNameclass 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| return _FILTER_COMPARISON_MAP[op](j_left, j_right) | ||
|
|
||
|
|
||
| def eq( |
There was a problem hiding this comment.
In the client you took care to handle case insensitivity for eq and ne. I think we also had matches and contains available.
There was a problem hiding this comment.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jmao-denver
force-pushed
the
DH-20658-complete-structured-filter-py
branch
from
f28bd08 to
c629e4f
Compare
2 participants
Note:
Expressionclass or theLiteralclass, in favor of the native Python typesFunctionandMethodclasses; workaround exists in the form of the class methodfrom_that takes in a raw string and returns a Filter object