Skip to content

Commit

Permalink
Fix parsing of multiple consecutive path wildcard, unpivot, path expr…
Browse files Browse the repository at this point in the history
…essions (#405)
  • Loading branch information
alancai98 authored Jul 6, 2023
1 parent 5629e88 commit 0f49d1b
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 37 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Add ability to parse `ORDER BY`, `LIMIT`, `OFFSET` in children of set operators

### Fixes
- Fixes parsing of multiple consecutive path wildcards (e.g. `a[*][*][*]`), unpivot (e.g. `a.*.*.*`), and path expressions (e.g. `a[1 + 2][3 + 4][5 + 6]`)—previously these would not parse correctly.

## [0.5.0] - 2023-06-06
### Changed
Expand Down
46 changes: 10 additions & 36 deletions partiql-parser/src/parse/partiql.lalrpop
Original file line number Diff line number Diff line change
Expand Up @@ -841,37 +841,11 @@ ExprPrecedence02: Synth<ast::Expr> = {
}

PathExpr: ast::Path = {
<l:ExprPrecedence01> "." <steps:PathSteps> => ast::Path { root:Box::new(l.data), steps },
<l:ExprPrecedence01> "[" "*" "]" "." <s:PathSteps> => {
<l:ExprPrecedence01> <s:PathSteps> => {
let step = ast::PathStep::PathWildCard;
ast::Path {
root: Box::new(l.data),
steps: std::iter::once(step).chain(s.into_iter()).collect()
}
},
<l:ExprPrecedence01> "[" <expr:ExprQuery> "]" "." <s:PathSteps> => {
let step = ast::PathStep::PathExpr(
ast::PathExpr{
index: Box::new(*expr),
}
);

ast::Path {
root: Box::new(l.data),
steps: std::iter::once(step).chain(s.into_iter()).collect()
}
},
<l:ExprPrecedence01> "[" "*" "]" => ast::Path {
root:Box::new(l.data), steps:vec![ast::PathStep::PathWildCard]
},
<l:ExprPrecedence01> "[" <expr:ExprQuery> "]" => {
let step = ast::PathStep::PathExpr(
ast::PathExpr{
index: Box::new(*expr),
});

ast::Path {
root:Box::new(l.data), steps:vec![step]
steps: s
}
},
}
Expand Down Expand Up @@ -1111,9 +1085,10 @@ FunctionArgName: ast::SymbolPrimitive = {
// Examples:
// a.b
// a.*
// a.[*]
// a[*]
// a[*][*]
// a.b.c
// a[*].b[*].c
// "a".b
// "a"."b"
// { 'a': 1, 'b': 2 }.a
Expand All @@ -1137,7 +1112,6 @@ PathSteps: Vec<ast::PathStep> = {
let mut steps = path;
steps.push(ast::PathStep::PathUnpivot);
steps
// ast::Path{ root:path.root, steps }
},
<lo:@L> <path:PathSteps> "[" <expr:ExprQuery> "]" <hi:@R> => {
let step = ast::PathStep::PathExpr(
Expand All @@ -1149,17 +1123,17 @@ PathSteps: Vec<ast::PathStep> = {
steps.push(step);
steps
},
"." <v:PathExprVarRef> => {
vec![ast::PathStep::PathExpr( ast::PathExpr{ index: Box::new(v) })]
},
"[" "*" "]" => {
vec![ast::PathStep::PathWildCard]
},
"[" <expr:ExprQuery> "]" => {
vec![ast::PathStep::PathExpr( ast::PathExpr{ index: Box::new(*expr) })]
},
"*" => {
"." "*" => {
vec![ast::PathStep::PathUnpivot]
},
<v:PathExprVarRef> => {
vec![ast::PathStep::PathExpr( ast::PathExpr{ index: Box::new(v) })]
"[" <expr:ExprQuery> "]" => {
vec![ast::PathStep::PathExpr( ast::PathExpr{ index: Box::new(*expr) })]
},
}

Expand Down

5 comments on commit 0f49d1b

@github-actions
Copy link

Choose a reason for hiding this comment

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

PartiQL (rust) Benchmark

Benchmark suite Current: 0f49d1b Previous: 5629e88 Ratio
parse-1 7583 ns/iter (± 507) 5870 ns/iter (± 159) 1.29
parse-15 66156 ns/iter (± 3197) 58046 ns/iter (± 1846) 1.14
parse-30 134079 ns/iter (± 11376) 109543 ns/iter (± 2610) 1.22
compile-1 6523 ns/iter (± 392) 5608 ns/iter (± 107) 1.16
compile-15 49290 ns/iter (± 3571) 41215 ns/iter (± 756) 1.20
compile-30 103258 ns/iter (± 4633) 82711 ns/iter (± 1861) 1.25
plan-1 128961 ns/iter (± 7451) 22839 ns/iter (± 491) 5.65
plan-15 2000121 ns/iter (± 79755) 428149 ns/iter (± 6372) 4.67
plan-30 3977293 ns/iter (± 207897) 837027 ns/iter (± 21035) 4.75
eval-1 30811155 ns/iter (± 1574887) 26140310 ns/iter (± 662213) 1.18
eval-15 160098924 ns/iter (± 3808858) 140488315 ns/iter (± 2297844) 1.14
eval-30 315382826 ns/iter (± 7469932) 267017773 ns/iter (± 4134560) 1.18
join 19250 ns/iter (± 915) 16619 ns/iter (± 385) 1.16
simple 9058 ns/iter (± 431) 8001 ns/iter (± 82) 1.13
simple-no 932 ns/iter (± 48) 770 ns/iter (± 15) 1.21
numbers 178 ns/iter (± 8) 170 ns/iter (± 2) 1.05
parse-simple 961 ns/iter (± 53) 868 ns/iter (± 4) 1.11
parse-ion 3011 ns/iter (± 103) 2709 ns/iter (± 40) 1.11
parse-group 10280 ns/iter (± 464) 8764 ns/iter (± 108) 1.17
parse-complex 28695 ns/iter (± 1025) 22901 ns/iter (± 351) 1.25
parse-complex-fexpr 39083 ns/iter (± 2649) 36577 ns/iter (± 596) 1.07

This comment was automatically generated by workflow using github-action-benchmark.

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'PartiQL (rust) Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite Current: 0f49d1b Previous: 5629e88 Ratio
plan-1 128961 ns/iter (± 7451) 22839 ns/iter (± 491) 5.65
plan-15 2000121 ns/iter (± 79755) 428149 ns/iter (± 6372) 4.67
plan-30 3977293 ns/iter (± 207897) 837027 ns/iter (± 21035) 4.75

This comment was automatically generated by workflow using github-action-benchmark.

CC: @alancai98 @alancai98 @partiql

@alancai98
Copy link
Member Author

Choose a reason for hiding this comment

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

Some local benchmarking comparing this commit and one before:

plan-1                  time:   [51.913 µs 61.437 µs 72.126 µs]
                        change: [+167.67% +201.32% +236.01%] (p = 0.00 < 0.05)
                        Performance has regressed.

plan-15                 time:   [336.50 µs 340.65 µs 345.91 µs]
                        change: [-1.0999% +0.6421% +2.5047%] (p = 0.49 > 0.05)
                        No change in performance detected.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

plan-30                 time:   [689.13 µs 699.69 µs 711.57 µs]
                        change: [-1.8742% +0.8143% +3.4221%] (p = 0.58 > 0.05)
                        No change in performance detected.

and some benchmarks provided by @jpschorr (comparing previous commit to new commit)

warning: `partiql-eval` (lib) generated 22 warnings (run `cargo fix --lib -p partiql-eval` to apply 18 suggestions)
    Finished bench [optimized + debuginfo] target(s) in 0.36s
     Running benches/bench_eval_multi_like.rs (target/release/deps/bench_eval_multi_like-20e35f15f9f6f696)
plan-1                  time:   [25.094 µs 25.411 µs 25.838 µs]
                        change: [-1.2730% +0.0642% +1.4567%] (p = 0.93 > 0.05)
                        No change in performance detected.
Found 16 outliers among 100 measurements (16.00%)
  1 (1.00%) low severe
  2 (2.00%) low mild
  5 (5.00%) high mild
  8 (8.00%) high severe

plan-15                 time:   [385.53 µs 386.58 µs 387.73 µs]
                        change: [-0.2456% +0.5888% +1.3235%] (p = 0.15 > 0.05)
                        No change in performance detected.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild

plan-30                 time:   [778.49 µs 780.72 µs 783.20 µs]
                        change: [-2.9345% -1.9226% -0.9954%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 11 outliers among 100 measurements (11.00%)
  2 (2.00%) low mild
  4 (4.00%) high mild
  5 (5.00%) high severe

The change in performance is within our range of performance change threshold. Appears to be a bad benchmark host but weird that the increased benchmarks occur with anything using planning.

@alancai98
Copy link
Member Author

Choose a reason for hiding this comment

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

Benchmarks appear to have returned back to normal with the subsequent commit to main: 139b276#commitcomment-120875230

@alancai98
Copy link
Member Author

Choose a reason for hiding this comment

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

Benchmarks are still elevated for the plan-* tests but down from their peek: https://partiql.org/partiql-lang-rust/dev/bench/

Please sign in to comment.