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

feat(expr): build expression from pretty string #8774

Merged
merged 7 commits into from
Mar 27, 2023

Conversation

wangrunji0408
Copy link
Contributor

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR introduces a simple DSL for expressions, so that we can build expressions painless in unit tests. (resolve #7881)

The language is defined as follows:

inputref := #<index>:<type>
constant := <value>:<type>
function := (<name>:<type> <child1> <child2>...)

For example:

#0 % 2 == 0
(equal:boolean (modulus:int8 #0:int8 2:int8) 0:int8)

The overall design follows the egg. Except that we have to explicitly specify the return type of each node. It might not be the best and is subject to be changed if anyone has a better idea.

This PR contains a small lexer and parser for the DSL (mainly generated by GPT), and replaces boilerplate code with the new language whenever possible.

Checklist For Contributors

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
  • All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

  • I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

  • My PR DOES NOT contain user-facing changes.

Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Copy link
Contributor

@stdrc stdrc left a comment

Choose a reason for hiding this comment

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

I would suggest $idx and int32/int64 instead of #idx and int4/int8, for better consistency with our current system. BTW, it's great to see this work!🚀 Guess we may need some documents for the new DSL.

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #8774 (327e3c3) into main (f4e2bdc) will decrease coverage by 0.03%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##             main    #8774      +/-   ##
==========================================
- Coverage   71.07%   71.05%   -0.03%     
==========================================
  Files        1167     1167              
  Lines      192447   192095     -352     
==========================================
- Hits       136783   136490     -293     
+ Misses      55664    55605      -59     
Flag Coverage Δ
rust 71.05% <88.46%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
src/common/src/array/stream_chunk.rs 92.10% <ø> (ø)
src/expr/src/expr/expr_some_all.rs 0.00% <ø> (ø)
src/expr/src/expr/mod.rs 48.71% <ø> (ø)
src/common/src/types/mod.rs 64.62% <69.23%> (+1.66%) ⬆️
src/expr/src/expr/build.rs 81.08% <81.08%> (ø)
src/batch/src/executor/filter.rs 75.51% <100.00%> (-8.28%) ⬇️
src/batch/src/executor/join/hash_join.rs 94.55% <100.00%> (-0.03%) ⬇️
src/batch/src/executor/join/local_lookup_join.rs 51.00% <100.00%> (-3.98%) ⬇️
src/batch/src/executor/join/nested_loop_join.rs 91.39% <100.00%> (-0.20%) ⬇️
src/batch/src/executor/sort_agg.rs 92.96% <100.00%> (-0.30%) ⬇️
... and 11 more

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

LGTM!! 🚀🚀🚀

src/common/src/array/data_chunk.rs Outdated Show resolved Hide resolved
@wangrunji0408
Copy link
Contributor Author

wangrunji0408 commented Mar 27, 2023

I would suggest $idx and int32/int64 instead of #idx and int4/int8, for better consistency with our current system.

All #idx have been replaced by $idx. ✅
But when I searched :int4 in our codebase, I found a lot of occurrence in SQL tests, e.g. select 1::int4.
Should we also be consistent with them? 🥲

@BugenZhao
Copy link
Member

+1 on int2 / int4. 🥵 It's better to follow the terminology of Postgres.

@stdrc
Copy link
Contributor

stdrc commented Mar 27, 2023

But when I searched :int4 in our codebase, I found a lot of occurrence in SQL tests, e.g. select 1::int4.
Should we also be consistent with them? 🥲

My fault 🥵, int2/int4 seems OK

Copy link
Contributor

@stdrc stdrc left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Merged via the queue into main with commit 7e592f3 Mar 27, 2023
@wangrunji0408 wangrunji0408 deleted the wrj/expr-from-pretty branch March 27, 2023 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test facility: build BoxedExpression from "pretty" representation of expression
4 participants