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

Update AdminScoreController.php #25

Merged
merged 1 commit into from May 1, 2017

Conversation

superyetkin
Copy link
Contributor

Optimized the SQL query to fetch user information

Optimized the SQL query to fetch user information
@codecov-io
Copy link

codecov-io commented Apr 29, 2017

Codecov Report

Merging #25 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   12.24%   12.24%   -0.01%     
==========================================
  Files          19       19              
  Lines        2523     2524       +1     
==========================================
  Hits          309      309              
- Misses       2214     2215       +1
Impacted Files Coverage Δ
src/AppBundle/Controller/AdminScoreController.php 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c15721...962886b. Read the comment docs.

$archiveTable = $lh->getTable("archive", $dbName);
$loggingTable = $lh->getTable("logging_userindex", $dbName);
$revisionTable = $lh->getTable("revision_userindex", $dbName);
$archiveTable = $lh->getTable("archive_userindex", $dbName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just broke backwards compatibility with all versions of MediaWiki. There's a reason we go through the labs helper - specifically to allow for a table mapping.

That should be documented better on my side, which is a separate issue.

SELECT 'blocks' as source, COUNT(*) as value FROM $loggingTable l
INNER JOIN $userTable u ON l.log_user = u.user_id
WHERE l.log_type='block' AND l.log_action='block'
AND l.log_namespace=2 AND l.log_deleted=0 AND u.user_name=:username
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... you attempt to make this more efficient by adding another Join - which is a slow and heavy process. That makes absolutely no sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the "log_user" column in the query makes it better despite adding another join.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm convinced. Please fix my other comment and we'll give this the thumbs up.

To clarify my other comment - in order to access a table you use the MediaWiki table rather than the labs table. We then map it using app/config/table_map.yml

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 from me. The new query definitely runs significantly faster, at around 21.73 sec run time with one test I did. The same test took over 3 minutes with the old query, I just ended up killing it.

Indeed Matthew is right however that we have a table mapping system in place. All you need to know is to pass the normal table name to $lh->getTable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean. Is there any chance you can make the necessary changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll merge then fix it up.

Thank you for this pull request 😄

@Matthewrbowker Matthewrbowker merged commit 9ee82d2 into x-tools:master May 1, 2017
@superyetkin superyetkin deleted the patch-1 branch May 1, 2017 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants