Skip to content

feat(common): add search expression #235

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 11, 2022
Merged

feat(common): add search expression #235

merged 1 commit into from
Feb 11, 2022

Conversation

@lmatz
Copy link
Contributor

@lmatz lmatz commented Feb 11, 2022 •
edited
Loading

What's changed and what's your intention?

Support in.

We have a new search function, which contains two arguments. The first one is typically an input ref expression, the second one is a set of values. This is how Calcite unifies in and between.

Sort of ugly on the java frontend side. But will keep modifying the code when supporting between later and other cases of in if there is any more.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

closes #192

@github-actions github-actions bot added the type/feature Type: New feature. label Feb 11, 2022
@lmatz lmatz force-pushed the lz/in branch 2 times, most recently from 7d01e07 to 7ff2823 Compare February 11, 2022 02:18
@lmatz lmatz mentioned this pull request Feb 11, 2022
Closed
2 tasks
@codecov
Copy link

codecov bot commented Feb 11, 2022 •
edited
Loading

Codecov Report

Merging #235 (46fd1ec) into main (27a4636) will decrease coverage by 0.05%.
The diff coverage is 59.78%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #235      +/-   ##
============================================
- Coverage     74.58%   74.52%   -0.06%     
- Complexity     2656     2657       +1     
============================================
  Files           841      848       +7     
  Lines         48063    48185     +122     
  Branches       1566     1573       +7     
============================================
+ Hits          35847    35911      +64     
- Misses        11416    11472      +56     
- Partials        800      802       +2
Flag Coverage Δ
java 61.93% <3.33%> (-0.12%) ⬇️
rust 79.94% <87.09%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rust/common/src/expr/mod.rs 36.66% <0.00%> (-1.27%) ⬇️
...lanner/rel/serialization/RexToProtoSerializer.java 58.57% <3.33%> (-7.93%) ⬇️
rust/common/src/expr/expr_search.rs 80.00% <80.00%> (ø)
rust/common/src/expr/build_expr_from_prost.rs 52.34% <100.00%> (+10.07%) ⬆️
rust/meta/src/manager/env.rs 84.00% <0.00%> (-8.00%) ⬇️
.../risingwave/planner/rel/streaming/RwStreamAgg.java 94.28% <0.00%> (-5.72%) ⬇️
rust/meta/src/hummock/hummock_manager.rs 87.41% <0.00%> (-2.43%) ⬇️
rust/stream/src/task/stream_manager.rs 52.21% <0.00%> (-0.92%) ⬇️
rust/meta/src/rpc/server.rs 90.00% <0.00%> (-0.17%) ⬇️
rust/meta/src/test_utils.rs 100.00% <0.00%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27a4636...46fd1ec. Read the comment docs.

@lmatz lmatz requested a review from fuyufjh February 11, 2022 02:41
@liurenjie1024
Copy link
Contributor

liurenjie1024 commented Feb 11, 2022 •
edited
Loading

In fact I prefer the solution in #170, which removes search operator. The search operator is ambiguous to me, and not quite unfriendly to vectorized execution. Also as @fuyufjh pointed out, this implementation is just in, not range sets.

@lmatz
Copy link
Contributor Author

lmatz commented Feb 11, 2022

We merge this now?

And rename SearchExpression to InExpression and add support for Between in the future pr.

@lmatz lmatz merged commit e21ca14 into main Feb 11, 2022
@lmatz lmatz deleted the lz/in branch February 11, 2022 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fuyufjh fuyufjh fuyufjh approved these changes

Assignees

No one assigned

Labels

type/feature Type: New feature.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support In

4 participants

@lmatz @liurenjie1024 @fuyufjh