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

Change modeling of project lists in logical plan #415

Merged
merged 1 commit into from
Jul 28, 2023
Merged

Conversation

alancai98
Copy link
Member

@alancai98 alancai98 commented Jul 26, 2023

Changes the modeling of partiql-logical's project to be a Vec rather than a HashMap. Previously, couldn't include multiple project aliases with the same name.

For some context, some conformance tests that will be fixed in an upcoming PR are partially failing because the select list included multiple expressions with the same alias.

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

@alancai98 alancai98 self-assigned this Jul 26, 2023
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (a43f057) 81.41% compared to head (c665bfc) 81.40%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
- Coverage   81.41%   81.40%   -0.01%     
==========================================
  Files          53       53              
  Lines       14914    14909       -5     
  Branches    14914    14909       -5     
==========================================
- Hits        12142    12137       -5     
  Misses       2251     2251              
  Partials      521      521              
Files Changed Coverage Δ
partiql-logical/src/lib.rs 40.46% <ø> (ø)
partiql-eval/src/eval/evaluable.rs 89.46% <100.00%> (ø)
partiql-eval/src/lib.rs 98.69% <100.00%> (ø)
partiql-eval/src/plan.rs 92.61% <100.00%> (ø)
partiql-logical-planner/src/lower.rs 85.73% <100.00%> (-0.06%) ⬇️

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

@github-actions
Copy link

Conformance comparison report

Base (a43f057) 257daa1 +/-
% Passing 85.99% 85.99% 0.00%
✅ Passing 5436 5436 0
❌ Failing 886 886 0
🔶 Ignored 0 0 0
Total Tests 6322 6322 0

Number passing in both: 5436

Number failing in both: 886

Number passing in Base (a43f057) but now fail: 0

Number failing in Base (a43f057) but now pass: 0

@alancai98 alancai98 requested review from jpschorr and am357 July 26, 2023 19:38
@alancai98
Copy link
Member Author

This was the conformance test that referenced multiple SELECT items that have the same alias: https://github.com/partiql/partiql-tests/blob/main/partiql-tests-data/eval/query/select/select-mysql.ion#L13776. This PR along with the fix for variable resolution in #410 will fix that test.

@alancai98 alancai98 merged commit 625aa08 into main Jul 28, 2023
@alancai98 alancai98 deleted the change-select-expr branch July 28, 2023 23:32
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.

3 participants