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

Look for COUNT instead of just first row in data_present_sensor #5897

Merged
merged 4 commits into from
Feb 20, 2023

Conversation

Thingus
Copy link
Contributor

@Thingus Thingus commented Feb 17, 2023

Closes #5090

I have:

  • Formatted any Python files with black
  • Brought the branch up to date with master
  • Added any relevant Github labels
  • Added tests for any new additions
  • Added or updated any relevant documentation
  • Added an Architectural Decision Record (ADR), if appropriate
  • Added an MPLv2 License Header if appropriate
  • Updated the Changelog

Description

Instead of SELECT ing the first row in staging_table in data_present_sensor, this PR uses COUNT to check for data presence. This would still fail if no records are present (COUNT (*) would return 0 in the first cell), but would work if there's any data present in the first 100 records (arbitarily selected)

@Thingus Thingus linked an issue Feb 17, 2023 that may be closed by this pull request
@Thingus Thingus marked this pull request as ready for review February 17, 2023 16:46
@Thingus Thingus added bug Something isn't working FlowETL Needs Review PR that is ready for review by a human labels Feb 17, 2023
@cypress
Copy link

cypress bot commented Feb 17, 2023

Passing run #18417 ↗︎

0 4 0 0 Flakiness 0

Details:

Changelog
Project: FlowAuth Commit: 9f72d53059
Status: Passed Duration: 00:47 💡
Started: Feb 20, 2023 9:57 AM Ended: Feb 20, 2023 9:58 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Member

@jc-harrison jc-harrison left a comment

Choose a reason for hiding this comment

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

Would be more efficient to do "select exists (select * from {{staging_table}} limit 1)"

@greenape
Copy link
Member

Approved - add a changelog before sticking ready to merge label on

@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Merging #5897 (9f72d53) into master (1bdc258) will decrease coverage by 14.85%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #5897       +/-   ##
===========================================
- Coverage   93.23%   78.39%   -14.85%     
===========================================
  Files         277      277               
  Lines       10828    10828               
  Branches      895      895               
===========================================
- Hits        10096     8489     -1607     
- Misses        602     2095     +1493     
- Partials      130      244      +114     
Impacted Files Coverage Δ
...etl/flowetl/flowetl/sensors/data_present_sensor.py 100.00% <ø> (ø)
...e/flowmachine/core/sqlalchemy_table_definitions.py 0.00% <0.00%> (-100.00%) ⬇️
...wmachine/features/subscriber/active_subscribers.py 0.00% <0.00%> (-100.00%) ⬇️
...ne/features/subscriber/per_subscriber_aggregate.py 0.00% <0.00%> (-100.00%) ⬇️
...e/features/utilities/unique_values_from_queries.py 0.00% <0.00%> (-100.00%) ⬇️
...owmachine/features/utilities/feature_collection.py 23.80% <0.00%> (-76.20%) ⬇️
flowmachine/flowmachine/core/geotable.py 28.57% <0.00%> (-71.43%) ⬇️
...flowmachine/features/subscriber/distance_series.py 30.00% <0.00%> (-70.00%) ⬇️
...e/flowmachine/features/subscriber/handset_stats.py 21.95% <0.00%> (-68.30%) ⬇️
...es/subscriber/contact_reference_locations_stats.py 33.33% <0.00%> (-66.67%) ⬇️
... and 103 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Thingus Thingus added ready-to-merge Label indicating a PR is OK to automerge and removed Needs Review PR that is ready for review by a human labels Feb 20, 2023
@mergify mergify bot merged commit d8777b9 into master Feb 20, 2023
@mergify mergify bot deleted the flowetl-null-bug branch February 20, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working FlowETL ready-to-merge Label indicating a PR is OK to automerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FlowETL DataPresentSensor trips over null fields
3 participants