Feature/expr parser #21
Conversation
ArnaudBger
commented
May 8, 2024
ArnaudBger
commented
- Add an expression parser in the substreams repo
- Add a new pb file for index keys
Cargo.toml
Outdated
| @@ -18,6 +18,9 @@ rust-version = "1.60" | |||
|
|
|||
| [workspace.dependencies] | |||
| substreams-macro = { version = "0.5.13", path = "./substreams-macro" } | |||
| pest= "2.7.10" | |||
| pest_derive = "2.7.10" | |||
| rstest = "0.19.0" | |||
There was a problem hiding this comment.
Is this just for testing, if yes, should go in dev-dependencies bucket
Cargo.toml
Outdated
| @@ -18,6 +18,9 @@ rust-version = "1.60" | |||
|
|
|||
| [workspace.dependencies] | |||
| substreams-macro = { version = "0.5.13", path = "./substreams-macro" } | |||
| pest= "2.7.10" | |||
There was a problem hiding this comment.
| pest= "2.7.10" | |
| pest = "2.7.10" |
substreams/Cargo.toml
Outdated
| pest= "2.7.10" | ||
| pest_derive = "2.7.10" | ||
| rstest = "0.19.0" |
There was a problem hiding this comment.
Pull from the workspace dependencies set
| pest= "2.7.10" | |
| pest_derive = "2.7.10" | |
| rstest = "0.19.0" | |
| pest = { workspace = true } | |
| pest_derive = { workspace = true } | |
| rstest = { workspace = true } |
substreams/tests/parser_test.rs
Outdated
| #[case(TEST_KEYS, "test1 && test2 ", true)] | ||
| #[case(TEST_KEYS, "test1 && test6", false)] |
There was a problem hiding this comment.
How do you validate that this is parsed correctly? It parses ok, but the resulting expression should be checked.
In the Go version, I created a string representation of the tree structure, so it can be verified too.
substreams/src/parser.rs
Outdated
| use pest::{iterators::Pair, Parser}; | ||
| use pest_derive::Parser; | ||
|
|
||
| #[derive(Parser)] |
There was a problem hiding this comment.
Can you check if it's possible to generate the code and commit it? It would be preferable leading to faster compiling for users since less time running the derive macro.
substreams/src/lib.rs
Outdated
| @@ -125,6 +125,7 @@ mod state; | |||
|
|
|||
| pub mod key; | |||
| pub mod store; | |||
| pub mod parser; | |||
There was a problem hiding this comment.
I don't like that it's called parser, parser doesn't say anything about what is being parsed.
Second, let's not publicly export the module, remove pub. We should only publicly expose the minimum we need for the library to be useful.
substreams/src/parser.rs
Outdated
| pub fn evaluate_expression(keys: Vec<String>, input: &str) -> bool { | ||
| let successful_parse = EParser::parse(Rule::expression, input).unwrap(); | ||
| return evaluate_rule(successful_parse.clone().into_iter().next().unwrap(), keys); | ||
| } | ||
|
|
||
| fn evaluate_rule(pair: Pair<Rule>, keys: Vec<String>) -> bool { | ||
| match pair.as_rule() { | ||
| Rule::expression => { | ||
| let inner_pair = pair.into_inner().next().unwrap(); | ||
| return evaluate_rule(inner_pair, keys); | ||
| } | ||
| Rule::or => { | ||
| let mut result = false; | ||
| for inner_pair in pair.into_inner() { | ||
| result = result || evaluate_rule(inner_pair, keys.clone()); | ||
| } | ||
| return result; | ||
| }, | ||
| Rule::and => { | ||
| let mut result = true; | ||
| for inner_pair in pair.into_inner() { | ||
| result = result && evaluate_rule(inner_pair, keys.clone()); | ||
| } | ||
| return result; | ||
| }, | ||
| Rule::value => { | ||
| let inner_pair = pair.into_inner().next().unwrap(); | ||
| return evaluate_rule(inner_pair, keys); | ||
| } | ||
| Rule::keyterm => { | ||
| return keys.contains(&pair.as_str().to_string()); | ||
| } | ||
| Rule::singleQuoteKeyTerm => { | ||
| return keys.contains(&pair.as_str().to_string().replace("'", "")); | ||
| } | ||
| Rule::doubleQuoteKeyTerm => { | ||
| return keys.contains(&pair.as_str().to_string().replace("\"", "")); | ||
| } | ||
| _ => {panic!("Unexpected rule encountered")} | ||
| } | ||
| } |
There was a problem hiding this comment.
You need to remove all the unwrap here and the panic. Transform the method to return a Result<bool, anyhow::Error>, all error code path should have a .context attached to the error so we now where it came from.
substreams/src/parser.rs
Outdated
| #[grammar = "parser_rule.pest"] | ||
| pub struct EParser; | ||
|
|
||
| pub fn evaluate_expression(keys: Vec<String>, input: &str) -> bool { |
There was a problem hiding this comment.
No need to receive a owned copy of a Vec<String>. We should accept variant of String like &str too. I would change to
| pub fn evaluate_expression(keys: Vec<String>, input: &str) -> bool { | |
| pub fn evaluate_expression<K: AsRef<str>, I: AsRef<str>>(keys: &[K], input: I) -> bool { |
And this can be used now with different variations of String, &String and &str:
evaluate_expression(&vec!["a", "b", "c"], "d");
evaluate_expression(
&vec!["a".to_string(), "a".to_string(), "a".to_string()],
"d",
);
let keys = vec!["a".to_string(), "a".to_string(), "a".to_string()];
let keys_ref = keys.iter().map(|x| x).collect::<Vec<&String>>();
evaluate_expression(&keys_ref, "d");
There was a problem hiding this comment.
Let's also find a more meaningful name for this function, it will be the only one exposed IMO so we better find it a very good clear name.
substreams/src/parser.rs
Outdated
|
|
||
| pub fn evaluate_expression(keys: Vec<String>, input: &str) -> bool { | ||
| let successful_parse = EParser::parse(Rule::expression, input).unwrap(); | ||
| return evaluate_rule(successful_parse.clone().into_iter().next().unwrap(), keys); |
There was a problem hiding this comment.
Let's avoid all .clone() in your code, there is a good chance that we can remove all of them.
