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

Convert covidcast signal OR clauses to UNION #1021

Closed
wants to merge 5 commits into from

Conversation

LeonLu2
Copy link
Collaborator

@LeonLu2 LeonLu2 commented Nov 2, 2022

closes #763

Prerequisites:

  • Unless it is a documentation hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

Summary

  1. Refactored the QueryBuilder to convert covidcast signal OR clauses to UNION
  2. Edited test_query.py accordingly to test filters changed in server code

@LeonLu2 LeonLu2 requested a review from krivard November 2, 2022 06:43
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

this is your first major change to the code, so I was pickier than I would otherwise be.

a few logic confirmations which need to be addressed before approval, but mostly style suggestions

also: are the filter_X equivalents to the new alternative_filter_X functions being used anymore? If not we can just drop them.

src/server/_query.py Outdated Show resolved Hide resolved
src/server/_query.py Outdated Show resolved Hide resolved
Comment on lines +187 to +189
if conditions:
for condition in conditions:
condition_array.append(f"({source_field} = :{source_param} AND {condition})")
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking my understanding: this block will always execute, since conditions will always evaluate true: alternative_filter_strings wraps alternative_filter_values, and alternative_filter_values returns either a nonempty string ("False") or a nonempty list (a function of pair.signal)

Is that right?

src/server/_query.py Outdated Show resolved Hide resolved
src/server/_query.py Outdated Show resolved Hide resolved
src/server/_query.py Outdated Show resolved Hide resolved
src/server/_query.py Outdated Show resolved Hide resolved
src/server/_query.py Show resolved Hide resolved
tests/server/test_query.py Show resolved Hide resolved
@LeonLu2 LeonLu2 requested a review from krivard November 5, 2022 04:44
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

I am reasonably satisfied, but I'm looping in Dmitry and George on this since it conflicts/interacts with a possible solution to a problem they're solving in an upcoming feature release for just-in-time processing.

@melange396
Copy link
Collaborator

The issue this was intended to address (#763) is no longer relevant, so i am closing this PR.

@melange396 melange396 closed this Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert covidcast signal OR clauses to UNION
3 participants