Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional structuring to Filter, Expression, and Literal #3441

Merged
merged 95 commits into from
May 4, 2023

Conversation

devinrsmith
Copy link
Member

@devinrsmith devinrsmith commented Feb 16, 2023

The interfaces of io.deephaven.api.expression.Filter, io.deephaven.api.expression.Expression, and io.deephaven.api.value.Value were originally developed and influenced based on the structure of the gRPC messages and engine capabilities; and they were explicitly called out due to lack of completeness with respect to these constructions in #829, #830, and #831.

Upon delving into SQL work in #3473, it became clear that it was time to update these interfaces with the more expressive structures that could better preserve the semantics and intentions of the SQL expressions. The alternative involves throwing up our hands and using io.deephaven.api.RawString everywhere instead.

For example, it was possible before to represent the filters of Foo > Bar and Foo > 42, but it was not possible to represent Foo > Bar + 42 (again, without dropping down into RawString); and equivalently, an Expression could be Bar, or 42, but not Bar + 42 (essentially, limiting what could be represented in the case of view or update Selectable).

This PR greatly expands the expressiveness of these interfaces while also cleaning up the hierarchy. The main highlights:

  • Filter is now an Expression
  • FilterCondition renamed FilterComparison
  • Value is gone; in its place, a more specific Literal structure
  • Usages of Value replaced with the more generic Expression type (for example, LHS / RHS of FilterComparison are now Expressions instead of Values)
  • ExpressionFunction created to represent function calls without needing to drop into RawString (for example, we might want to have io.deephaven.sql.Functions#sum_sql as something that can be properly referenced).

This PR does not aim to update the gRPC layer at this time, mainly do to time considerations and that #3473 is limited to execution completely within the server environment. That said, we would likely benefit from updating the gRPC layer to be more expressive in the ways proposed here.

There's also potential in the future for the engine layer to benefit from the additional structuring here; potentially constructing more advanced filters and expressions without needing to compile strings.

@devinrsmith devinrsmith added this to the Feb 2023 milestone Feb 16, 2023
@devinrsmith devinrsmith self-assigned this Feb 16, 2023
@devinrsmith devinrsmith requested a review from chipkent May 1, 2023 14:40
chipkent
chipkent previously approved these changes May 1, 2023
@devinrsmith devinrsmith added ReleaseNotesNeeded Release notes are needed and removed NoReleaseNotesNeeded No release notes are needed. labels May 1, 2023

public abstract Expression expression();

public abstract List<Literal> values();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RHS of a match filter can be a QueryScope param. Literal is insufficiently inclusive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay - I'll expand this type b/c I agree. That said, there is room for a QueryScope extends Expression type in the future that we don't currently have.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to support FilterMatches in some way, but it could be via equals, in, and case-insensitive String-specific variants of each (4 total).
I still think you need param support.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should follow-up later w/ #3784

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And #3785

return ImmutableFilterMatches.builder();
}

public abstract Expression expression();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expression seems too general. Why not ColumnName?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're trying to be as generic enough to capture the semantics that one could reasonably want to express (or express via SQL), even though our engine might not support it for now.

As a quasi example in our parser-like syntax:

Foo + Bar in "foobar"

inverted);
}
// Delegate to comparisons
return of(inverted ? in.inverseAsComparisons() : in.asComparisons());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to go to a ConditionFilter in this case; if we do a disjunction, we'll load the inputs N times.

@devinrsmith devinrsmith requested a review from rcaudy May 3, 2023 23:54
@devinrsmith devinrsmith merged commit 6ab8787 into deephaven:main May 4, 2023
@devinrsmith devinrsmith deleted the filter-condition-expression branch May 4, 2023 14:21
@deephaven-internal
Copy link
Contributor

@github-actions github-actions bot locked and limited conversation to collaborators May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants