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

Improve peopleLocations endpoint: people_locations method #3217

Merged
merged 11 commits into from Aug 14, 2018

Conversation

Projects
None yet
5 participants
@milaaraujo
Copy link
Collaborator

milaaraujo commented Aug 8, 2018

Hey everybody!

We are opening this request to fix the issue #3196. Here we implemented a new version of recentPeople method.

Now we first select the users that have lat/lon and then we join the users table with the revisions table to order them by most recent activities. We also change the name of the method from recentPeople to people_locations.

We would like some feedback, thanks! :)

fixes #3196
@publiclab/reviewers @publiclab/soc @jywarren

@plotsbot

This comment has been minimized.

Copy link
Collaborator

plotsbot commented Aug 8, 2018

1 Warning
⚠️ It looks like you merged from master in this pull request. Please rebase to get rid of the merge commits – you may want to rewind the master branch and rebase instead of merging in from master, which can cause problems when accepting new code!
1 Message
📖 @milaaraujo 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.

Generated by 🚫 Danger

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 8, 2018

This looks great. I'd love a review from one other person maybe? @publiclab/reviewers

user_scope = User.where('rusers.status <> 0')\
.joins(:user_tags)\
.where('value LIKE "lat:%"')\
.joins(:revisions)\

This comment has been minimized.

@jywarren

jywarren Aug 8, 2018

Contributor

This might be a bit heavy if all you're wanting is the timestamp. I wonder if you could select just the timestamp, but you might have to do .select('node_revisions.timestamp', plus every other column (Not sure if you can wildcard * multiple columns...) so then the way to get past that could be to get the absolute minimum number of columns with a select(), collect the nids with nids = query.collect(&:nid) and then run a second query, like:

User.where(nid: nids)

This way the original query is very narrowly restricted, then a second query collects the full User records. This is a bit messy looking but you can see how it might result in a more efficient initial query followed by a narrower second query which gets all the User table bits and pieces you're looking for, without the messy joins. Does this make sense?

This comment has been minimized.

@milaaraujo

milaaraujo Aug 9, 2018

Author Collaborator

To be honest I don't know if it makes sense! 😝

Actually, under rails, the query being executed is: SELECT DISTINCT "rusers".* FROM "rusers" INNER JOIN "user_tags" ON "user_tags"."uid" = "rusers"."id" INNER JOIN "node" ON "node"."uid" = "rusers"."id" INNER JOIN "node_revisions" ON "node_revisions"."nid" = "node"."nid" WHERE (rusers.status <> 0) AND (value LIKE "lat:%") ORDER BY node_revisions.timestamp DESC LIMIT ?. We are only selecting the data from the user table, which is what we need at the end of the day, and we use the timestamp just to sort the results. Of course, I do not know exactly how mysql runs this query, but it is unlikely that it actually returns the entire secondary table to only then select what the query explicitly asks for...

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Aug 9, 2018

But what I'm saying is that we are not returning every column of the revisions table. We are returning -> SELECT DISTINCT "rusers".*. And I really don't know how mysql runs the joins, but I believe it has its own optimization.

However, @namangupta01 what do you think, you were recently thinking about a query optimization? [2] 😄

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

I think I'm at the limit of my knowledge of mysql too! I thought that joining essentially adds new columns for each of the joined tables columns. But I'm trying to test this to see what the raw msyql result is, and whether we're getting revision body content in the results... hang on a sec and apologies!

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

Hmm:

> user_scope = User.where('rusers.status <> 0').joins(:user_tags).where('value LIKE "lat:%"').joins(:revisions).order("node_revisions.timestamp DESC")
# ActiveRecord::ConfigurationError: Can't join 'User' to association named 'revisions'; perhaps you misspelled it?
@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

Oh wait, duh - has_many :revisions, through: :node -- adding this...

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

I see the query as:

  User Load (17.8ms)  SELECT  `rusers`.* FROM `rusers` INNER JOIN `user_tags` ON `user_tags`.`uid` = `rusers`.`id` INNER JOIN `node` ON `node`.`uid` = `rusers`.`id` INNER JOIN `node_revisions` ON `node_revisions`.`nid` = `node`.`nid` WHERE (rusers.status <> 0) AND (value LIKE "lat:%") ORDER BY node_revisions.timestamp DESC LIMIT 11

let me think...

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

OK, full SQL output (just running the generated query in the command-line mysql) is:

+----+----------+-------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+-------------+--------------------+---------------------+------------------+---------------+------------------+---------------+---------------------+---------------------+-------------------+-------+-----------+-----------------+--------------------+-----------------+------+--------------+--------+
| id | username | email             | crypted_password                                                                                                                 | password_salt                                                                                                                    | persistence_token                                                                                                                | login_count | failed_login_count | last_request_at     | current_login_at | last_login_at | current_login_ip | last_login_ip | created_at          | updated_at          | openid_identifier | role  | reset_key | photo_file_name | photo_content_type | photo_file_size | bio  | token        | status |
+----+----------+-------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+-------------+--------------------+---------------------+------------------+---------------+------------------+---------------+---------------------+---------------------+-------------------+-------+-----------+-----------------+--------------------+-----------------+------+--------------+--------+
|  1 | Bob      | bob@publiclab.org | 4d7b3086073fdc9d7647f598d9e036e0e6e210b680bf6ea402631ae28afc1a92d8c9640923ed9d8d738c341b264ae147aa06bbf4024fc319ab76554fbd3d0b52 | a517e2d46393a2a1ae1fd29cc5153abee965973ed572f72bbeca977d5b81be51fdbb8a115b6d4ace6c6cefa05fecd2f5dbf57669092613cfa734b4b41c66ee82 | 0f4d6908dc76f59302b57fd35b77661a2938d7481727f611086e1ab347669337bb122e32b4b3f905f1547fb29725c10cf7133455cf8f2492888e67e64187b2f0 |           0 |                  0 | 2018-01-26 01:24:55 | NULL             | NULL          | NULL             | NULL          | 2018-01-26 01:24:55 | 2018-01-26 01:24:55 | NULL              | basic | NULL      | NULL            | NULL               | NULL            |      | abcdefg12345 |      1 |
+----+----------+-------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+-------------+--------------------+---------------------+------------------+---------------+------------------+---------------+---------------------+---------------------+-------------------+-------+-----------+-----------------+--------------------+-----------------+------+--------------+--------+
1 row in set (0.00 sec)
@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

Oddly, it seems different when I do this:

User.where('rusers.status <> 0').joins(:user_tags).where('value LIKE "lat:%"').joins(:revisions).order("node_revisions.timestamp DESC").select('rusers.*, node_revisions.timestamp').limit(2)
  User Load (12.4ms)  SELECT  rusers.*, node_revisions.timestamp FROM `rusers` INNER JOIN `user_tags` ON `user_tags`.`uid` = `rusers`.`id` INNER JOIN `node` ON `node`.`uid` = `rusers`.`id` INNER JOIN `node_revisions` ON `node_revisions`.`nid` = `node`.`nid` WHERE (rusers.status <> 0) AND (value LIKE "lat:%") ORDER BY node_revisions.timestamp DESC LIMIT 2

That outputs:

+----+----------+-------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+-------------+--------------------+---------------------+------------------+---------------+------------------+---------------+---------------------+---------------------+-------------------+-------+-----------+-----------------+--------------------+-----------------+------+--------------+--------+------------+
| id | username | email             | crypted_password                                                                                                                 | password_salt                                                                                                                    | persistence_token                                                                                                                | login_count | failed_login_count | last_request_at     | current_login_at | last_login_at | current_login_ip | last_login_ip | created_at          | updated_at          | openid_identifier | role  | reset_key | photo_file_name | photo_content_type | photo_file_size | bio  | token        | status | timestamp  |
+----+----------+-------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+-------------+--------------------+---------------------+------------------+---------------+------------------+---------------+---------------------+---------------------+-------------------+-------+-----------+-----------------+--------------------+-----------------+------+--------------+--------+------------+
|  1 | Bob      | bob@publiclab.org | 4d7b3086073fdc9d7647f598d9e036e0e6e210b680bf6ea402631ae28afc1a92d8c9640923ed9d8d738c341b264ae147aa06bbf4024fc319ab76554fbd3d0b52 | a517e2d46393a2a1ae1fd29cc5153abee965973ed572f72bbeca977d5b81be51fdbb8a115b6d4ace6c6cefa05fecd2f5dbf57669092613cfa734b4b41c66ee82 | 0f4d6908dc76f59302b57fd35b77661a2938d7481727f611086e1ab347669337bb122e32b4b3f905f1547fb29725c10cf7133455cf8f2492888e67e64187b2f0 |           0 |                  0 | 2018-01-26 01:24:55 | NULL             | NULL          | NULL             | NULL          | 2018-01-26 01:24:55 | 2018-01-26 01:24:55 | NULL              | basic | NULL      | NULL            | NULL               | NULL            |      | abcdefg12345 |      1 | 1522444233 |
|  1 | Bob      | bob@publiclab.org | 4d7b3086073fdc9d7647f598d9e036e0e6e210b680bf6ea402631ae28afc1a92d8c9640923ed9d8d738c341b264ae147aa06bbf4024fc319ab76554fbd3d0b52 | a517e2d46393a2a1ae1fd29cc5153abee965973ed572f72bbeca977d5b81be51fdbb8a115b6d4ace6c6cefa05fecd2f5dbf57669092613cfa734b4b41c66ee82 | 0f4d6908dc76f59302b57fd35b77661a2938d7481727f611086e1ab347669337bb122e32b4b3f905f1547fb29725c10cf7133455cf8f2492888e67e64187b2f0 |           0 |                  0 | 2018-01-26 01:24:55 | NULL             | NULL          | NULL             | NULL          | 2018-01-26 01:24:55 | 2018-01-26 01:24:55 | NULL              | basic | NULL      | NULL            | NULL               | NULL            |      | abcdefg12345 |      1 | 1522444233 |
+----+----------+-------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+----------------------------------------------------------------------------------------------------------------------------------+-------------+--------------------+---------------------+------------------+---------------+------------------+---------------+---------------------+---------------------+-------------------+-------+-----------+-----------------+--------------------+-----------------+------+--------------+--------+------------+
@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

hmm, things get stranger but maybe clearer?...


2.4.1 :004 > User.group('rusers.id').where('rusers.status <> 0').joins(:user_tags).where('value LIKE "
lat:%"').joins(:revisions).order("node_revisions.timestamp DESC").select('rusers.*, node_revisions.timestamp').limit(2)
  User Load (6.6ms)  SELECT  rusers.*, node_revisions.timestamp FROM `rusers` INNER JOIN `user_tags` ON `user_tags`.`uid` = `rusers`.`id` INNER JOIN `node` ON `node`.`uid` = `rusers`.`id` INNER JOIN `node_revisions` ON `node_revisions`.`nid` = `node`.`nid` WHERE (rusers.status <> 0) AND (value LIKE "lat:%") GROUP BY rusers.id ORDER BY node_revisions.timestamp DESC LIMIT 2                                  
ActiveRecord::StatementInvalid: Mysql2::Error: Expression #25 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'plots.node_revisions.timestamp' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by: SELECT  rusers.*, node_revisions.timestamp FROM `rusers` INNER JOIN `user_tags` ON `user_tags`.`uid` = `rusers`.`id` INNER JOIN `node` ON `node`.`uid` = `rusers`.`id` INNER JOIN `node_revisions` ON `node_revisions`.`nid` = `node`.`nid` WHERE (rusers.status <> 0) AND (value LIKE "lat:%") GROUP BY rusers.id ORDER BY node_revisions.timestamp DESC LIMIT 2
@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

Hmm, so I think i'm now stuck. In response to the error in the last one, i think it's saying we need to add node_revisions to our group_by, but that's silly because then it wouldn't group by User and we get duplicate records (which I can confirm from running this locally).

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

I guess I suspect the original version without the extra select() may not have really been sorting by timestamp. Looking at this attempt to get a distinct response:

2.4.1 :022 > User.where('rusers.status <> 0').joins(:user_tags).where('value LIKE "lat:%"').joins(:revisions).order("node_revisions.timestamp DESC").limit(2).distinct
  User Load (2.3ms)  SELECT  DISTINCT `rusers`.* FROM `rusers` INNER JOIN `user_tags` ON `user_tags`.`uid` = `rusers`.`id` INNER JOIN `node` ON `node`.`uid` = `rusers`.`id` INNER JOIN `node_revisions` ON `node_revisions`.`nid` = `node`.`nid` WHERE (rusers.status <> 0) AND (value LIKE "lat:%") ORDER BY node_revisions.timestamp DESC LIMIT 2                                                                    
ActiveRecord::StatementInvalid: Mysql2::Error: Expression #1 of ORDER BY clause is not in SELECT list, references column 'plots.node_revisions.timestamp' which is not in SELECT list; this is incompatible with DISTINCT: SELECT  DISTINCT `rusers`.* FROM `rusers` INNER JOIN `user_tags` ON `user_tags`.`uid` = `rusers`.`id` INNER JOIN `node` ON `node`.`uid` = `rusers`.`id` INNER JOIN `node_revisions` ON `node_revisions`.`nid` = `node`.`nid` WHERE (rusers.status <> 0) AND (value LIKE "lat:%") ORDER BY node_revisions.timestamp DESC LIMIT 2
@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

This is hard!!! 😭

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

OK, we're also affected by this: https://dev.mysql.com/doc/refman/5.7/en/group-by-handling.html

I'm going to take a short break here, probs a good idea!

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 9, 2018

OK, thinking thinking. Although we could potentially solve this, our intent is to return people who are recently active.

Thinking outside the box, what if we simply sorted by user.last_request_at or user.last_login_at ?

@jywarren jywarren referenced this pull request Aug 9, 2018

Closed

Profile api test PR #3224

@wafflebot wafflebot bot added the in progress label Aug 10, 2018

@stefannibrasil

This comment has been minimized.

Copy link
Collaborator

stefannibrasil commented Aug 10, 2018

I checked here, we are using this version of mariadb:

+-----------------+
| VERSION()       |
+-----------------+
| 10.2.16-MariaDB |
+-----------------+
@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Aug 10, 2018

I think the last commit will solve the problem for mysql! 🤞

But Travis is failing with this new weird error:

ERROR: Service 'web' failed to build: The command '/bin/sh -c bundle install --jobs 4' returned a non-zero code: 5

😩

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 10, 2018

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 10, 2018

Looks like a timeout issue, maybe travis servers lost internet connection for a bit. Restarting!

fatal: unable to access 'https://github.com/publiclab/inline-markdown-editor.git/': Failed to connect to github.com port 443: Connection timed out

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Aug 10, 2018

My last commit didn't work on Travis, so I'm removing the changes.

@milaaraujo milaaraujo force-pushed the milaaraujo:improve-peopleLocations-endpoint branch from 67bd2e5 to 9aa3ef5 Aug 11, 2018

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Aug 11, 2018

Hey Jeff, now I'm running mysql locally and I could reproduce the error you were getting. That fix I tried early really solved the problem for mysql... But it failed on Travis.

1 - This query works for mariaDB, Sqlite and Travis:

User.where('rusers.status <> 0')\
    .joins(:user_tags)\
    .where('value LIKE "lat:%"')\
    .joins(:revisions)\
    .order("node_revisions.timestamp DESC")\
    .distinct

2 - This one works for mysql:

User.select('rusers.*,node_revisions.timestamp')
    .where('rusers.status <> 0')\
    .joins(:user_tags)\
    .where('value LIKE "lat:%"')\
    .joins(:revisions)\
    .order("node_revisions.timestamp DESC")\
    .distinct

3 - So I'm trying now this logic (last commit). It works for mysql and sqlite, but failed on Travis:

if ActiveRecord::Base.connection.adapter_name == 'Mysql2'
	User.select('rusers.*,node_revisions.timestamp')
        .where('rusers.status <> 0')\
        .joins(:user_tags)\
        .where('value LIKE "lat:%"')\
        .joins(:revisions)\
        .order("node_revisions.timestamp DESC")\
        .distinct
else
    User.where('rusers.status <> 0')\
        .joins(:user_tags)\
        .where('value LIKE "lat:%"')\
        .joins(:revisions)\
        .order("node_revisions.timestamp DESC")\
        .distinct
end

The conclusion today is that it is not possible to make everyone happy! My question now is which database does Travis use and which database is used in production? If a code passes without errors by Travis, does that mean it will run smoothly in production? If this is true, can we leave the version 2 that works on Travis?

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Aug 11, 2018

Hey, @jywarren! More insights here about this... I'm using mysql to run the site locally and a lot of things (not related to our work) are broken, for example when I try to create a new question:

screenshot from 2018-08-11 15-15-59

So I don't think we are using mysql in production, right? I think it makes more sense to keep the version that works on Travis, what do you think?

@icarito

This comment has been minimized.

Copy link
Member

icarito commented Aug 12, 2018

Just a note that we are using Mariadb 10.2 in production. Not sure why you are seeing this issue! Thanks for your hard work!

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Aug 12, 2018

Hey @icarito! I was testing locally with mysql, because Jeff pointed out he was seeing some errors in my query using mysql... But then I discovered that other things also break when I test with mysql.

Now I'm focusing on leaving the code error-free testing with MariaDB (which is what's in production). My last commit seems to be working okay!

Now that Jeff is on vacation, could you please review it for me? 😄

@milaaraujo milaaraujo requested a review from icarito Aug 12, 2018

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 12, 2018

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 13, 2018

OK, going through this now. First, on the includes() solution, you can leave out the final .collect(&:username) and it'll return the full results. Ah, i see you figured that out.

While includes() does select a lot, it is just more detailed in aliasing all the selected columns, and to my understanding of SQL, the results will include all the same columns. It's a long dense query when Rails writes it all out and makes all the aliases, but the effective response from the database is the same. I may be wrong about this! But that's my best understanding and the resources I've read show that includes() (using a different join type) is not necessarily less efficient than joins().

Does includes() work everywhere? If so let's use it. Otherwise, let's open a new issue to very clearly tell everyone to use mariadb and not mysql and then go ahead with your solution that works in maridab -- i totally agree with your assessment.

In general it's nice to use generic-enough SQL features that it'll work in all similar database systems. But we're already pushing this a bit since we use full-text search, for example. So if includes() doesn't work, I'm OK with formally adopting mariadb and dropping support for mysql.

Thanks for looking through this so carefully! A very evidence-driven decision making process. If you try includes() and it doesn't work, just tell me when you're ready with the mariadb version (is it 3d7c5e4 ?)

Thanks!!!

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 13, 2018

(also just want to note this error here for future reference; e4a72e5 produces the following):

- [ ] There was a test error at:  Failure: test_search_recent_people_functionality_having_specified_tagName(Minitest::Result) [/usr/local/bundle/gems/mysql2-0.5.2/lib/mysql2/client.rb:131]: ActiveRecord::StatementInvalid: Mysql2::Error: Operand should contain 1 column(s): SELECT 'rusers'.* FROM 'rusers' INNER JOIN 'user_tags' ON 'user_tags'.'uid' = 'rusers'.'id' WHERE (user_tags.value LIKE 'tool:barometer') AND ('rusers'.'id') IN (SELECT DISTINCT rusers.*,node_revisions.timestamp, rusers.id FROM 'rusers' INNER JOIN 'user_tags' ON 'user_tags'.'uid' = 'rusers'.'id' INNER JOIN 'node' ON 'node'.'uid' = 'rusers'.'id' INNER JOIN 'node_revisions' ON 'node_revisions'.'nid' = 'node'.'nid' WHERE (rusers.status <> 0) AND (value LIKE "lat:%") ORDER BY node_revisions.timestamp DESC) LIMIT 100 app/services/search_service.rb:178:in 'people_locations' app/services/execute_search.rb:23:in 'execute' app/services/execute_search.rb:3:in 'by' app/api/srch/search.rb:112:in 'execute' app/api/srch/search.rb:100:in 'block (2 levels) in <class:Search>' <a href='https://github.com/milaaraujo/plots2/tree/improve-peopleLocations-endpoint/test/functional/search_api_test.rb#L214'>test/functional/search_api_test.rb:214</a>:in 'block in <class:SearchApiTest>' 
@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Aug 13, 2018

Hey @jywarren! I know that the includes() solution definitely works, I tried it before! The problem with it is not all the aliases it creates, the problem is that it selects/returns all the columns of the revision table:

SELECT DISTINCT "rusers"."id" AS t0_r0, "rusers"."username" AS t0_r1, "rusers"."email" AS t0_r2, "rusers"."crypted_password" AS t0_r3, "rusers"."password_salt" AS t0_r4, "rusers"."persistence_token" AS t0_r5, "rusers"."login_count" AS t0_r6, "rusers"."failed_login_count" AS t0_r7, "rusers"."last_request_at" AS t0_r8, "rusers"."current_login_at" AS t0_r9, "rusers"."last_login_at" AS t0_r10, "rusers"."current_login_ip" AS t0_r11, "rusers"."last_login_ip" AS t0_r12, "rusers"."created_at" AS t0_r13, "rusers"."updated_at" AS t0_r14, "rusers"."openid_identifier" AS t0_r15, "rusers"."role" AS t0_r16, "rusers"."reset_key" AS t0_r17, "rusers"."photo_file_name" AS t0_r18, "rusers"."photo_content_type" AS t0_r19, "rusers"."photo_file_size" AS t0_r20, "rusers"."bio" AS t0_r21, "rusers"."token" AS t0_r22, "rusers"."status" AS t0_r23, "rusers"."password_checker" AS t0_r24, **"node_revisions"."vid" AS t1_r0, "node_revisions"."nid" AS t1_r1, "node_revisions"."uid" AS t1_r2, "node_revisions"."title" AS t1_r3, "node_revisions"."body" AS t1_r4, "node_revisions"."teaser" AS t1_r5, "node_revisions"."log" AS t1_r6, "node_revisions"."timestamp" AS t1_r7, "node_revisions"."format" AS t1_r8, "node_revisions"."status" AS t1_r9** FROM "rusers" INNER JOIN "user_tags" ON "user_tags"."uid" = "rusers"."id" LEFT OUTER JOIN "node" ON "node"."uid" = "rusers"."id"
 LEFT OUTER JOIN "node_revisions" ON "node_revisions"."nid" = "node"."nid" WHERE (rusers.status <> 0) AND (value LIKE "lat:%") AND ("rusers"."id") IN (?, ?) ORDER BY node_revisions.timestamp DESC [["id", 6], ["id", 5]]

The joins() solution only selects/returns the user table:

SELECT DISTINCT "rusers".* FROM "rusers" INNER JOIN "user_tags" ON "user_tags"."uid" = "rusers"."id" INNER JOIN "node" ON "node"."uid" = "rusers"."id" INNER JOIN "node_revisions" ON "node_revisions"."nid" = "node"."nid" WHERE (rusers.status <> 0) AND (value LIKE "lat:%") ORDER BY node_revisions.timestamp DESC LIMIT ?

So the effective response from the database is not exactly the same. If you don't think this is a problem, I can certainly send a new commit (using includes())! 😃

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Aug 13, 2018

And I see you are worried about the support for mysql. But I don't think at this point, considering the code we have in production now, we really don't support mysql. I run rake test yesterday in the master branch, using mysql, and it pointed out 43 errors! Did you try to do this before? Or maybe I'm using a wrong mysql version... Sorry to be pushy about it! But I'm just trying to understand what's going on.

desenho sem titulo

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 14, 2018

milaaraujo added some commits Aug 14, 2018

@milaaraujo

This comment has been minimized.

Copy link
Collaborator Author

milaaraujo commented Aug 14, 2018

@jywarren, done.

@jywarren jywarren merged commit be06b33 into publiclab:master Aug 14, 2018

3 checks passed

codeclimate 1 fixed issue
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger ⚠️ 1 Warning. Don't worry, everything is fixable.
Details

@wafflebot wafflebot bot removed the in progress label Aug 14, 2018

@jywarren

This comment has been minimized.

Copy link
Contributor

jywarren commented Aug 14, 2018

Fantastic!

@milaaraujo milaaraujo deleted the milaaraujo:improve-peopleLocations-endpoint branch Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.