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

855 monitor intersection movements for missing high volume movements #1025

Conversation

gabrielwol
Copy link
Collaborator

@gabrielwol gabrielwol commented Jul 26, 2024

What this pull request accomplishes:

  • adds a view to find common movements in the last 100 days which are not in intersection_movements table
    • add a weekly notification to check this view (see example in #airflow_pipelines-dev)
  • Adds a denylist: intersection_movements_denylist with same structure as base ("allowlist")
    • Didn't want to add this as column to existing table because it would require a lot more join changes
  • Adds BEFORE INSERT triggers on both intersection_movements and intersection_movements_denylist to restrict inserting overlapping rows between the two. See it in action below.
  • Note there is a new after insert trigger (to insert padding values based on new inserts to intersection_movements) being developed in Miovision Aggregation Improvements (ATR view, intersection_movements AFTER INSERT trigger, unacceptable_gaps) #1024 which will make it easier to make additions to intersection_movements.

Test new triggers using:

INSERT INTO miovision_api.intersection_movements
SELECT * FROM miovision_api.intersection_movements_denylist LIMIT 1;

INSERT INTO miovision_api.intersection_movements_denylist
SELECT * FROM miovision_api.intersection_movements LIMIT 1;

Issue(s) this solves:

What, in particular, needs to reviewed:

  • The two functions are basically the same, maybe I should make them take a table parameter...

What needs to be done by a sysadmin after this PR is merged

E.g.: these tables need to be migrated/created in the production schema.

Copy link
Collaborator

@chmnata chmnata left a comment

Choose a reason for hiding this comment

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

just some questions on the view, possibility of using 1 function for two tables 🤔

SUM(v.volume) / ict.volume::numeric AS volume_frac
FROM miovision_api.volumes AS v
--anti join intersection_movements_denylist
LEFT JOIN miovision_api.intersection_movements_denylist AS im_dl
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we are missing a where im_dl.blahblah is null in the where clause for the anti join

@@ -0,0 +1,59 @@
CREATE OR REPLACE VIEW miovision_api.monitor_intersection_movements AS (
Copy link
Collaborator

Choose a reason for hiding this comment

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

when I select from the view, there was one row where sum_volume > intersection_classification_total which might be sus.
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not what I exepcted! It's because SELECT now()::date AT TIME ZONE 'Canada/Eastern' - interval ' 100 days' results in 2024-05-21 20:00:00 so the larger value had an extra 4 hrs of data.


DECLARE row_in_im numeric = (
SELECT COUNT(*)
FROM miovision_api.intersection_movements_denylist AS im
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to make the table name a param?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Copy link
Collaborator

@chmnata chmnata left a comment

Choose a reason for hiding this comment

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

:gabe-approved:

@gabrielwol gabrielwol force-pushed the 855-monitor-intersection_movements-for-missing-high-volume-movements branch from 28a0bcf to 3408371 Compare September 3, 2024 21:06
@gabrielwol gabrielwol force-pushed the 855-monitor-intersection_movements-for-missing-high-volume-movements branch from 3408371 to af8c29d Compare September 3, 2024 21:10
@gabrielwol gabrielwol merged commit 6d60239 into master Sep 3, 2024
5 checks passed
@gabrielwol gabrielwol deleted the 855-monitor-intersection_movements-for-missing-high-volume-movements branch September 3, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to monitor intersection_movements for missing high volume movements
2 participants