search

debrief / pepys-import

Support library for Pepys maritime data analysis environment
https://pepys-import.readthedocs.io/
Apache License 2.0
5 stars 5 forks source link

Sensor/Platform and Source ids to be treated as tuples in where clauses of Measurement functions instead of being considered individually #916

Open rnllv opened 3 years ago

rnllv commented 3 years ago

Version

Current

🐞 Description

The Measurement functions and queries currently used in Debrief Import consider Sensor Ids, Platform Ids, and Source Ids as independent search filters along with Start Time, End Time, and Location filters. Start Time, End Time, and Location filters can be treated individually, but Sensor Id/Platform Ids and Source Ids have to be treated together.

This where clause is wrong:

where starttime = .. , endtime = .., location = ...,
sensor_id in ('sensor1','sensor2') and 
source_id in ('source1','source2')

This is how the where clause should be:

where starttime = .. , endtime = .., location = ...,
(sensor_id, source_id) in (('sensor1','source1'),('sensor1','source2'),('sensor2','source1'))

⚙️ Current behavior

The current version of the query will load data for ('sensor2','source2') combination as well.

⚙️ Expected behavior

The query should only load data for selected combination of sensors and source ids.

🔢 Steps to Reproduce

🖥️ Screenshots

rnllv commented 3 years ago

@IanMayo,

Kindly confirm if Measurement functions will be used outside Debrief as well. Because, it doesn't seem tenable to maintain the null filters in the query.

Right now, we're checking individual filters and if they're null, the respective column is not filtered. But when it comes to tuple, this behaviour might be costly to achieve.

IanMayo commented 3 years ago

Hello @arunlal-v - yes, let's fix the query.

robintw commented 3 years ago

Do you know what the status of this is @IanMayo? The last comment on https://github.com/debrief/debrief/issues/5107 says that it has been completed, but there doesn't seem to have been any change to the .sql files in the Pepys repo. Any ideas?

IanMayo commented 3 years ago

I Think I recall the issue. I Think it's when we have multiple filters (area and time), Pepys returns any data that matches the area, plus any data that matches the time period. I think this restructuring returns data that meets both criteria.

Let me have a deeper check tomorrow.

robintw commented 3 years ago

I'm concerned that this PR is still open, and could reflect a fairly large bug with how Debrief and Pepys interact. Is there any chance you could look into this before I go @IanMayo?

robintw commented 3 years ago

@IanMayo I see this is down as one of my essential tasks, but I'm not really sure of the status or what needs doing. Could you look into it?

IanMayo commented 3 years ago

Sure. I think your involvement is only if I require an alembic migration for a new stored procedure. I'll remove backlog label, and re-attach it if/when there is something to do.