Skip to content

Conversation

LeopoldArkham
Copy link
Contributor

@LeopoldArkham LeopoldArkham commented Jan 29, 2017

Fixes #495

I swapped the redundant arrow link on the right for the download count, I don't think anybody used it (?).

@LeopoldArkham
Copy link
Contributor Author

@carols10cents Ok, it works now. I had to abuse the CI because the tests won't run on my machine.
Let me know how it looks.

@carols10cents
Copy link
Member

@LeopoldArkham Hm, why doesn't the ordering in SQL work?

@LeopoldArkham
Copy link
Contributor Author

LeopoldArkham commented Feb 3, 2017

@carols10cents Whenever a query contains a DISTINCT ON statement and an ORDER BY clause, the leftmost argument to ORDER BY has to be the same as the one to DISTINCT.

So since we're DISTINCT ON crate_name, I can only ORDER BY crate_name. I think it would be possible to do in all SQL with a sub-query, but this seemed like a clearer and more maintainable way.

If there's a policy against this (or I'm missing something), I can go back and look into it.

(Didn't mean to close)

@carols10cents
Copy link
Member

Oh! There's no policy or anything, it's just that SQL is really good at ordering things really fast :)

So the reason a reverse dependency might be duplicated is if a crate lists another crate as a dependency multiple times, such as target-specific dependencies. So for crate A, it might depend on crate B twice, so crate B's reverse dependencies would have duplicate listing of A. However, in both listings of A, the number of crates.downloads for A will be the same. So we should be able to add crate_downloads to the list of columns specified in the DISTINCT clause, and meet the necessary criteria for the ordering, like this:

SELECT DISTINCT ON (crate_downloads, crate_name)
    dependencies.*,
    crates.downloads AS crate_downloads,
    crates.name AS crate_name
FROM dependencies
INNER JOIN versions
ON versions.id = dependencies.version_id
INNER JOIN crates
ON crates.id = versions.crate_id
WHERE dependencies.crate_id = 2
AND versions.num = crates.max_version
ORDER BY crate_downloads DESC;

What do you think? Is there anything I'm missing? (it's entirely possible!)

@LeopoldArkham
Copy link
Contributor Author

No, you're totally right, thank you! I had misunderstood the behavior of DISTINCT ON when provided with more than 1 argument. (See? No programming Sunday evenings.)

I also got the test suite to (mostly) work, so we should be OK.

@LeopoldArkham
Copy link
Contributor Author

^ I jinxed myself.
Although it's the yarn install that failed. Gonna close and re-open to trigger a rebuild.

@carols10cents
Copy link
Member

Looks great, works like a charm!!! 🍀 Sorry I took so long to review this, thank you for your patience!! ❤️

@carols10cents carols10cents merged commit a8be938 into rust-lang:master Feb 20, 2017
@LeopoldArkham
Copy link
Contributor Author

And you for yours!

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.

2 participants