Nuanda / smogmapper

Smog Mapper
http://smogmapper.smogathon.pl/
1 stars 0 forks source link

Map - get only current sensor locations for markers #42

Closed Nuanda closed 8 years ago

Nuanda commented 8 years ago

(already discussed in #34 ; also remember not to return a sensor without any location)

mkasztelnik commented 8 years ago

@Nuanda, @nowakowski, @kammerer any idea how to convert this query http://stackoverflow.com/questions/2111384/sql-join-selecting-the-last-records-in-a-one-to-many-relationship into active record query?

mkasztelnik commented 8 years ago

Correct pure SQL query to solve this issue is as follow:

SELECT s.id, l1.latitude, l1.longitude 
FROM sensors s 
JOIN locations l1 ON (s.id = l1.sensor_id) 
LEFT OUTER JOIN locations l2 ON (s.id = l2.sensor_id AND (l1.registration_time < l2.registration_time OR l1.registration_time = l2.registration_time AND l1.id < l2.id)) 
WHERE l2.id IS NULL;
mkasztelnik commented 8 years ago

I added one additional commit into #40 since my commit requires changes introduced in this PR. If anyone know how to make joins condition more active record like please let me know.

Nuanda commented 8 years ago

@mkasztelnik You can put sql join code inside the joins AR method parameter - it's still chainable then.

mkasztelnik commented 8 years ago

That is what I done :) Take a look at final version: https://github.com/Nuanda/smogmapper/blob/location_registration/app/models/sensor.rb#L17-L28.

Nuanda commented 8 years ago

Hmm, don't you think the js code on the client side requires only sensor_id and both geo coords? Could it be that we can solve that by querying Location model only?

mkasztelnik commented 8 years ago

Right now you are right querying location should be enough. Do you know if additional data from sensor (e.g. marker type - not it is hard coded in js, sensor capability, you name it) will not be needed?

Nuanda commented 8 years ago

Might be - if your query is fast for the current testing data, then we may use it just as well.

Nuanda commented 8 years ago

For Location-only query, this is probably sth like that: http://stackoverflow.com/questions/5391564/how-to-use-distinct-and-order-by-in-same-select-statement

mkasztelnik commented 8 years ago

It is quite fast:

[3] pry(main)> Sensor.count
   (2.2ms)  SELECT COUNT(*) FROM "sensors"
=> 341
[4] pry(main)> Location.count
   (0.3ms)  SELECT COUNT(*) FROM "locations"
=> 342
[5] pry(main)> require 'benchmark'
=> false
[6] pry(main)> puts Benchmark.measure { Sensor.with_last_location }
  0.000000   0.000000   0.000000 (  0.000243)
``
Nuanda commented 8 years ago

Please include in the PR and also adjust the coffeescript where required.

mkasztelnik commented 8 years ago

Can you specify what to include? I made changes needed to solve this issue as a part of #40 PR since there are changes in Sensor-Location objects required to solve it.

Nuanda commented 8 years ago

I though it should go in #34 - since there is the new location model. What changes do you mean?

mkasztelnik commented 8 years ago

There is this commit https://github.com/Nuanda/smogmapper/commit/b596c2827f9e0932ab2186a3de8e9381c31ab20f and I wanted to avoid merge conflicts...

If you want I can move my commits into token_and_locations branch but it would require pushing with force into location_registration branch to remove commits specific to last sensor location.

Nuanda commented 8 years ago

You are free to modify this code as long as it returns more or less the same amount of data.