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

Base duplicate index on column names #4600

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Conversation

rauchy
Copy link
Contributor

@rauchy rauchy commented Jan 30, 2020

What type of PR is this? (check all applicable)

  • Bug Fix

Description

I've noticed that duplicate column names are indexed according to the amount of duplicates, and not based off the actual column name duplicates. For example: if you join the users table with the organizations table you would see name, created_at, updated_at, name1, created_at2, updated_at3 where I would expect name, created_at, updated_at, name1, created_at1, updated_at1.

Not sure if this is intentional, and also it will probably break existing queries, so feel free to reject this :)

@rauchy rauchy requested a review from arikfr January 30, 2020 08:04
@rauchy
Copy link
Contributor Author

rauchy commented Jan 30, 2020

Duplicate of #4353.

@rauchy rauchy closed this Jan 30, 2020
@rauchy
Copy link
Contributor Author

rauchy commented Jan 30, 2020

Following the edge cases described in #4353, I believe 03d7836 should be a better solution.

@guidopetri
Copy link
Contributor

@rauchy , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

@justinclift
Copy link
Member

justinclift commented Jul 22, 2023

Attempted a manual merge just now using the web interface. No idea if it'll work, and it'll probably fail the format check. But, lets see what happens... 😄

@justinclift
Copy link
Member

justinclift commented Jul 22, 2023

Just fixed the formatting with make format.

This PR still needs to be reviewed though. 😄

@codecov
Copy link

codecov bot commented Jul 22, 2023

Codecov Report

Merging #4600 (a03969f) into master (e18cd8f) will increase coverage by 0.02%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4600      +/-   ##
==========================================
+ Coverage   60.74%   60.77%   +0.02%     
==========================================
  Files         154      154              
  Lines       12628    12629       +1     
  Branches     1716     1716              
==========================================
+ Hits         7671     7675       +4     
+ Misses       4732     4730       -2     
+ Partials      225      224       -1     
Files Changed Coverage Δ
redash/query_runner/__init__.py 77.14% <100.00%> (+1.02%) ⬆️

... and 1 file with indirect coverage changes

@guidopetri
Copy link
Contributor

I think we should merge this one instead of #4353 , since this one fully encompasses the other PR. I also think we might want a test here before merging

@guidopetri guidopetri mentioned this pull request Jul 22, 2023
1 task
@justinclift
Copy link
Member

Good thinking @guidopetri, yep we'll want some kind of test. 😄

@guidopetri guidopetri self-assigned this Jul 29, 2023
@guidopetri guidopetri enabled auto-merge (squash) August 24, 2023 03:52
@guidopetri guidopetri merged commit 2d6f5b0 into master Aug 24, 2023
15 checks passed
@justinclift justinclift deleted the column-name-based-duplicates branch August 24, 2023 04:22
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