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.

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

feat: PromQL Rust Lexer #12

Merged
merged 23 commits into from
Dec 21, 2022
Merged

feat: PromQL Rust Lexer #12

merged 23 commits into from
Dec 21, 2022

Conversation

yuanbohan
Copy link
Contributor

@yuanbohan yuanbohan commented Dec 15, 2022 •
edited
Loading

Sorry, but this is a huge PR, and it's hard to review. Most of the work is to convert PromQL Lexer Go version into idiomatic Rust version.

What's included

  • PromQL Lexer Rust version
  • no pretty print supported, just parse the input string into an Lexeme Iterator
  • most of the test cases based on lex_test.go (only some cases left with FIXME comment)

What't to do next

  • Parser with unit tests

@yuanbohan yuanbohan marked this pull request as ready for review December 16, 2022 07:26
@codecov
Copy link

codecov bot commented Dec 16, 2022 •
edited
Loading

Codecov Report

Merging #12 (3ee5940) into develop (7addaf1) will increase coverage by 37.85%.
The diff coverage is 90.07%.

@@             Coverage Diff              @@
##           develop      #12       +/-   ##
============================================
+ Coverage    47.80%   85.65%   +37.85%     
============================================
  Files           11       11               
  Lines          341     1464     +1123     
============================================
+ Hits           163     1254     +1091     
- Misses         178      210       +32
Impacted Files Coverage Δ
src/label/matcher.rs 53.44% <ø> (ø)
src/parser/parse.rs 0.00% <0.00%> (ø)
src/parser/token.rs 23.91% <23.75%> (+23.91%) ⬆️
src/parser/lex.rs 95.52% <95.52%> (+95.52%) ⬆️
src/parser/production.rs 100.00% <100.00%> (+100.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@evenyag
Copy link
Contributor

evenyag commented Dec 16, 2022

I think I need a few days to review this PR, as I also need to compare this with the lexer in go.

Does the title need to follow the conventional commits rules? We have an action for it.

@yuanbohan
Copy link
Contributor Author

yuanbohan commented Dec 16, 2022 •
edited
Loading

I think I need a few days to review this PR, as I also need to compare this with the lexer in go.

Does the title need to follow the conventional commits rules? We have an action for it.

Yeah. I have integrated this action in #13

@yuanbohan yuanbohan changed the title Lexer feat: PromQL Lexer Rust Version Dec 16, 2022
evenyag
evenyag reviewed Dec 18, 2022
View reviewed changes
Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

I haven't checked the details of the lexer, so most comments are related to the code structure and styles.

src/parser/lex.rs Show resolved Hide resolved
src/parser/lex.rs Show resolved Hide resolved
src/parser/lex.rs Show resolved Hide resolved
src/parser/lex.rs Outdated Show resolved Hide resolved
src/parser/lex.rs Outdated Show resolved Hide resolved
src/parser/lex.rs Outdated Show resolved Hide resolved
src/parser/token.rs Outdated Show resolved Hide resolved
src/parser/token.rs Show resolved Hide resolved
src/parser/lex.rs Show resolved Hide resolved
src/parser/lex.rs Outdated Show resolved Hide resolved
@evenyag
Copy link
Contributor

evenyag commented Dec 18, 2022 •
edited
Loading

This repo might need a codecov.yml file to turn off the coverage warning.
image

@yuanbohan
Copy link
Contributor Author

Actually comments from @evenyag is greatly helpful. 👍

@yuanbohan
Copy link
Contributor Author

This repo might need a codecov.yml file to turn off the coverage warning. image

I planed to do the parse mod with unit test this week, leave the warning here will remind me to cover it.

@yuanbohan
misc(lex): modification based on review comments
07348ac 
- don't expose some public methods
- refactor test cases

Reviewer: @evenyag
Refs: #14 #15
evenyag
evenyag reviewed Dec 19, 2022
View reviewed changes
src/parser/lex.rs Outdated Show resolved Hide resolved
src/parser/lex.rs Outdated Show resolved Hide resolved
evenyag
evenyag reviewed Dec 19, 2022
View reviewed changes
src/parser/lex.rs Outdated Show resolved Hide resolved
@yuanbohan
misc(lex): revision based on review comment
0a6cbe0 
- func in token mod, directly use match arms
- use char default method, instead of a hashmap
- use for loop to asset_eq!(left, right) instead of assert!(all)

reviewer: @evenyag
evenyag
evenyag reviewed Dec 19, 2022
View reviewed changes
src/parser/token.rs Show resolved Hide resolved
src/parser/token.rs Outdated Show resolved Hide resolved
@yuanbohan yuanbohan changed the title feat: PromQL Lexer Rust Version feat: PromQL Rust Lexer Rust Dec 20, 2022
@yuanbohan yuanbohan changed the title feat: PromQL Rust Lexer Rust feat: PromQL Rust Lexer Dec 20, 2022
waynexia
waynexia approved these changes Dec 21, 2022
View reviewed changes
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I've skimmed the code and tests. They are very concrete and look good to me 👍

src/parser/token.rs Show resolved Hide resolved
src/parser/lex.rs Outdated Show resolved Hide resolved
evenyag
evenyag approved these changes Dec 21, 2022
View reviewed changes
Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

I haven't compared the codes with the Go version line by line, as I think we should guarantee it by adding more test cases to ensure we have the same behavior as the promql's lexer. Thus I decide to approve this PR.

@evenyag evenyag merged commit a5f2822 into develop Dec 21, 2022
@waynexia waynexia deleted the lexer branch December 21, 2022 08:22
waynexia pushed a commit that referenced this pull request Feb 13, 2023
@yuanbohan @waynexia
feat: PromQL Rust Lexer (#12)
c982299 
* feat(lex): skeleton

* refactor(lexer): use enum instead of trait object

* lex: keyword_or_identifier

* fix(ctx): pos, start, peek logic

* refactor: fn -> Fn

* refactor: move shift into Lexer

* lexer: accept_string

* lexer: duration, lable_selector

* example: make example work with lexer

* test: common test done

* test(lex): more test cases

* test(lex): unbalanced parentheses

* test(lexer): subquery

* test(lexer): more cases

* test: token and production fn

* misc(lex): modification based on review comments

- don't expose some public methods
- refactor test cases

Reviewer: @evenyag
Refs: #14 #15

* misc(lex): revision based on review comment

- func in token mod, directly use match arms
- use char default method, instead of a hashmap
- use for loop to asset_eq!(left, right) instead of assert!(all)

reviewer: @evenyag

* misc(lex): use const instead of lazy_static

* perf(token): use hashmap instead of match on &str

* fix(lex): potential panic try to dec depth when depth is less than 1

* misc(lex): use usize instead of u8 for paren_depth

* feat(lex): to check if backup is at the beginning of input

* comment(lex): typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@waynexia waynexia waynexia approved these changes

@evenyag evenyag evenyag approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuanbohan @evenyag @waynexia