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

Trending tags showing duplicates [WIP] #4263

Closed
wants to merge 1 commit into from

Conversation

vatbq
Copy link
Contributor

@vatbq vatbq commented Dec 11, 2018

Fixes #3963
I'm sure this works now! Check it out for you
screen shot 2018-12-11 at 1 30 05 am

I saw that the last time it didn't work so I would like try again :) Thank you!

@vatbq
Copy link
Contributor Author

vatbq commented Dec 11, 2018

@jywarren

@plotsbot
Copy link
Collaborator

2 Messages
📖 @ValentinaTironi Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 Pull Request is marked as Work in Progress

Generated by 🚫 Danger

@milaaraujo
Copy link
Collaborator

@ValentinaTironi, I assigned #3188 to myself and leave a comment saying I would work on it 3 days ago. Don’t start working on something that another person is also working on without warning beforehand. This prevents us from wasting our time...

@milaaraujo
Copy link
Collaborator

Anyway I can review this later, I already have a few examples ready on my computer to test the problem.

@vatbq
Copy link
Contributor Author

vatbq commented Dec 11, 2018

@milaaraujo I'm so sorry, I didn't see your comment :/ Maybe we can work together, as you prefer. So sorry for inconveniences, I had to ask before do the pull request. Really sorry :)

@jywarren
Copy link
Member

Hi, I just wanted to say this can happen, it's all right - and in this case, it's an interesting outcome because the solution proposed here was different. I think we have a potential path forward in #3188 but I am interested in whether the distinct being at the end is resulting in a different query that also works?

Thanks all for cooperating and your work is always valued here 👍 🙌

@milaaraujo
Copy link
Collaborator

It is ok, @ValentinaTironi! It was not my intention to discourage anyone from collaborating with the project! ;) Any collaboration is welcome, but it is always good to try to coordinate our work in order to make the most of each one.

Your solution is also working properly:

SELECT  DISTINCT `term_data`.`name` FROM `term_data` 
INNER JOIN `community_tags` ON `community_tags`.`tid` = `term_data`.`tid` 
INNER JOIN `community_tags` `node_tags_term_data_join` ON `node_tags_term_data_join`.`tid` = `term_data`.`tid` 
INNER JOIN `node` ON `node`.`nid` = `node_tags_term_data_join`.`nid` 
WHERE (node.status = 1) AND (node.created > 1541995976) AND (node.created <= 1544587976) 
GROUP BY `term_data`.`name`, node.nid, term_data.tid, community_tags.nid, community_tags.uid, community_tags.date 
ORDER BY count DESC LIMIT 5;
+------------------------+
| name                   |
+------------------------+
| first-time-poster      |
| lat:74.92979837188261  |
| lat:35.67314305234031  |
| lon:21.714137445587394 |
| lon:45.76772749324683  |
+------------------------+

I just think the solution in #4266 is better because it is simpler and removes "group by" that does not make much sense here. What do you think?

@vatbq
Copy link
Contributor Author

vatbq commented Dec 12, 2018

I'm totally agree 💯 I left a comment in you PR already #4266

@milaaraujo
Copy link
Collaborator

After merging #4266 we will have an extra step to test it with MySQL:

"then let's actually open a PR where we change the database back to mysql (but version 5.7) to see if the error crops up there again. [...]"

You can help with this if you want!

@vatbq
Copy link
Contributor Author

vatbq commented Dec 12, 2018

That is a more efficient solution

@vatbq vatbq closed this Dec 12, 2018
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.

4 participants