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

restore aliases before annotating (#27637) #27663

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

dpsutton
Copy link
Contributor

Backport and fixes #27464

  • restore aliases before annotating

  • cleanup

  • fix tests

  • Don't add escaped->original if aliases have not changed

No need to walk and replace the aliases if they are identical. And in that case, no need to keep a mapping of identical to identical. Not super important but saves some time and complexity, and keeps other tests passing since the presence of [:info :alias/escaped->original] in the query caused them to trivially fail.

  • oracle has a smaller limit and expected mangled

previous testing behavior was "what happened" and not what should happen. we fixed the bug but the "expect garbage" behavior was still present

  • Relax :alias/escaped->original schema

oracle tests use keywords for the alias

{:alias/escaped->original
 {:test-data-venues--via-a763718f "test_data_venues__via__venue_id",
  :test-data-users--via--user-id "test_data_users__via__user_id"}}}

No idea why that is keyworded

  • relax :alias/escaped->original schema

see previous commit

Before submitting the PR, please make sure you do the following
  • If you're attempting to fix a translation issue, please submit your changes to our POEditor project instead of opening a PR.

Tests

  • Run the frontend and Cypress end-to-end tests with yarn lint && yarn test)
  • If there are changes to the backend codebase, run the backend tests with clojure -X:dev:test
  • Sign the Contributor License Agreement
    (unless it's a tiny documentation change).

* restore aliases before annotating

* cleanup

* fix tests

* Don't add escaped->original if aliases have not changed

No need to walk and replace the aliases if they are identical. And in
that case, no need to keep a mapping of identical to identical. Not
super important but saves some time and complexity, and keeps other
tests passing since the presence of [:info :alias/escaped->original] in
the query caused them to trivially fail.

* oracle has a smaller limit and _expected_ mangled

previous testing behavior was "what happened" and not what should
happen. we fixed the bug but the "expect garbage" behavior was still
present

* Relax :alias/escaped->original schema

oracle tests use keywords for the alias

```clojure
{:alias/escaped->original
 {:test-data-venues--via-a763718f "test_data_venues__via__venue_id",
  :test-data-users--via--user-id "test_data_users__via__user_id"}}}
```

No idea why that is keyworded

* relax `:alias/escaped->original` schema

see previous commit
@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Base: 64.40% // Head: 64.40% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (9c8aa86) compared to base (4b41465).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@               Coverage Diff               @@
##           release-x.45.x   #27663   +/-   ##
===============================================
  Coverage           64.40%   64.40%           
===============================================
  Files                3169     3169           
  Lines               93108    93120   +12     
  Branches            11806    11807    +1     
===============================================
+ Hits                59963    59973   +10     
- Misses              28406    28407    +1     
- Partials             4739     4740    +1     
Flag Coverage Δ
back-end 85.49% <100.00%> (-0.01%) ⬇️
front-end 45.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
shared/src/metabase/mbql/schema.cljc 99.47% <100.00%> (+<0.01%) ⬆️
...c/metabase/query_processor/middleware/annotate.clj 82.10% <100.00%> (-0.13%) ⬇️
...query_processor/middleware/escape_join_aliases.clj 93.33% <100.00%> (-1.27%) ⬇️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dpsutton dpsutton merged commit c9312cb into release-x.45.x Jan 13, 2023
@dpsutton dpsutton deleted the manual-backport-alternative-fix-aliases branch January 13, 2023 14:58
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.

None yet

2 participants