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

Variable resolution and dynamic lookup fixes #416

Merged
merged 17 commits into from
Aug 10, 2023
Merged

Conversation

alancai98
Copy link
Member

@alancai98 alancai98 commented Jul 26, 2023

Closes #411.

Variable resolution and dynamic lookup would sometimes perform the incorrect lookup order or do too many lookups (e.g. do a global lookup on every local lookup). This PR

  • modifies the variable resolution node in logical plan to distinguish between the local and global lookup
  • fixes the group by data flow in name resolution

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

github-actions bot commented Jul 26, 2023

Conformance comparison report

Base (59a5fb1) f1419c0 +/-
% Passing 88.46% 88.40% -0.06%
✅ Passing 5595 5607 12
❌ Failing 730 736 6
🔶 Ignored 0 0 0
Total Tests 6325 6343 18

Number passing in both: 5595

Number failing in both: 722

Number passing in Base (59a5fb1) but now fail: 0

Number failing in Base (59a5fb1) but now pass: 8

The following test(s) were previously failing but now pass. Before merging, confirm they are intended to pass:

Click here to see
  • partiql_tests::eval::query::select::select::select_mysql::select::strict_mysql_select_29
  • partiql_tests::eval::query::select::select::select::select_join::permissive_join_with_shadowed_global
  • partiql_tests::eval::query::select::select::select::select_join::strict_join_with_shadowed_global
  • partiql_tests::eval::query::select::select::select_mysql::select::strict_mysql_select_23
  • partiql_tests::eval::query::select::select::select_mysql::select::permissive_mysql_select_23
  • partiql_tests::eval::query::select::select::select::select_join::strict_select_non_correlated_join
  • partiql_tests::eval::query::select::select::select_mysql::select::permissive_mysql_select_29
  • partiql_tests::eval::query::select::select::select::select_join::permissive_select_non_correlated_join

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Patch coverage: 91.52% and project coverage change: +0.07% 🎉

Comparison is base (59a5fb1) 81.74% compared to head (2761709) 81.82%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #416      +/-   ##
==========================================
+ Coverage   81.74%   81.82%   +0.07%     
==========================================
  Files          62       62              
  Lines       15426    15548     +122     
  Branches    15426    15548     +122     
==========================================
+ Hits        12610    12722     +112     
- Misses       2303     2307       +4     
- Partials      513      519       +6     
Files Changed Coverage Δ
...tension/partiql-extension-ion-functions/src/lib.rs 77.86% <0.00%> (-0.50%) ⬇️
partiql-conformance-test-generator/src/reader.rs 0.00% <0.00%> (ø)
partiql-types/src/lib.rs 68.75% <0.00%> (ø)
partiql-logical/src/lib.rs 40.57% <50.00%> (+0.10%) ⬆️
partiql-eval/src/eval/expr/path.rs 94.93% <84.21%> (-3.58%) ⬇️
partiql-ast-passes/src/name_resolver.rs 82.58% <85.24%> (-0.12%) ⬇️
partiql-logical-planner/src/lower.rs 86.40% <91.83%> (+0.59%) ⬆️
partiql-eval/src/eval/evaluable.rs 90.21% <100.00%> (-0.02%) ⬇️
partiql-eval/src/eval/expr/operators.rs 90.86% <100.00%> (ø)
partiql-eval/src/eval/expr/pattern_match.rs 88.75% <100.00%> (ø)
... and 4 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alancai98 alancai98 changed the title WIP -- update path step query context WIP -- variable resolution and dynamic lookup fixes Jul 26, 2023
Comment on lines -9 to -10
RUST_TEST_TIME_UNIT: 150,5000
RUST_TEST_TIME_INTEGRATION: 150,5000
Copy link
Member Author

Choose a reason for hiding this comment

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

A couple of the MYSQL tests were failing because they were taking a bit over these time thresholds.

);
}
Err(_) => {}
if let Ok(plan) = plan {
Copy link
Member Author

Choose a reason for hiding this comment

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

Applying some clippy suggestions.

@alancai98 alancai98 changed the title WIP -- variable resolution and dynamic lookup fixes Variable resolution and dynamic lookup fixes Jul 28, 2023
@alancai98 alancai98 marked this pull request as ready for review July 28, 2023 00:44
Base automatically changed from change-select-expr to main July 28, 2023 23:32
@alancai98 alancai98 force-pushed the change-var-resolution branch from 8055cfa to a94edc0 Compare July 31, 2023 23:51
@alancai98
Copy link
Member Author

Conformance test regressions (#416 (comment)) are due to updated partiql-tests submodule, which corrected the behavior in some MOD builtin function tests. The behavior is corrected in #413.

@alancai98 alancai98 force-pushed the change-var-resolution branch from 638b959 to bda363e Compare August 9, 2023 21:57
@alancai98 alancai98 requested a review from jpschorr August 9, 2023 22:22
@alancai98 alancai98 merged commit 2cbc7a4 into main Aug 10, 2023
@alancai98 alancai98 deleted the change-var-resolution branch August 10, 2023 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Variable resolution and dynamic lookup evaluation incorrect or performing too many lookups
2 participants