Implemented wizard query of road vs entire country motorcycle accidents #1624
Conversation
…infographics-staging.web.app/) that will be display as part of our DEMO, in this commit you will find: *Fix google OAuth *Disable facebook OAuth - compatibility with the current code in "https://anyway-infographics-staging.web.app/" *This commit broke - "ANYWAY Administration Panel" and the user personal preferences *This code replace the users table in the DB with user_oauth table *Disable login from anyway.co.il
…class. In the API, lang is not mandatory, defaults to 'heb'.lang not passed to cache creation, assuming we go for localization after cache.
…rison, issue 1579.
There was a problem hiding this comment.
The related code seems good, but there are some things that need attention:
- There is a lot of unrelated code in this pull request(git troubles?) - some of it maybe old code that will override the existing code
- For the code that I understood to be related to the pull request - I added some notes
- It seems that you have some conflicts and so the automatic linter and tests didn't run
| vehicle_other = "אחר" | ||
| vehicle_motorcycle = "אופנוע" | ||
| case_vehicle = case([( | ||
| InvolvedMarkerView.involve_vehicle_type.in_([8, 10, 19, 9]), literal_column(f"'{vehicle_motorcycle}'") |
There was a problem hiding this comment.
It usually recommended to use consts and not magic numbers - as the numbers may change over time and so we will need to find all related code to change them, also it will be easier for new volunteer to read the code.
The array that you are using here is identical to the one in BackEndConstants.MOTORCYCLE_VEHICLE_TYPES
| query = get_query(table_obj=InvolvedMarkerView, filters={}, start_time=start_time, | ||
| end_time=end_time) | ||
|
|
||
| query_columns = [InvolvedMarkerView.accident_id, InvolvedMarkerView.provider_and_id, |
There was a problem hiding this comment.
I didn't find a use for some of those columns(like InvolvedMarkerView.provider_code) in the original SQL query - what are their purpose here?
There was a problem hiding this comment.
InvolvedMarkerView.provider_code is used in order by (original query stated it as 3)
There was a problem hiding this comment.
The number 3 in the ORDER BY 3 DESC is used to point the newly created percentage column.
You can read more about it in:
ORDER BY the numbers
There was a problem hiding this comment.
I'm quite sure it refers to the internal query, not the external one (it's within the parentheses). See here as it will highlight the parenthses:
https://app.redash.io/hasadna/queries/511391/source
There was a problem hiding this comment.
Yes, you are right -
Sorry for the incorrect comment.
| .group_by(*query_columns) | ||
| .order_by(desc(InvolvedMarkerView.provider_code)) |
There was a problem hiding this comment.
When the SQL query is showing :
... GROUP BY 1,2 ORDER BY 3 DESC ...
they mean:
... GROUP BY LOCATION, vehicle ORDER BY percentage DESC ...
The number is the column by order in our SQL query SELECT phase.
Was the change intentional?
There was a problem hiding this comment.
I believe the ordering refers to the internal query
There was a problem hiding this comment.
Yes, you are right.
The correct ordering is:
1 - LOCATION
2 - vehicle
3 - num_of_accidents
| .filter(InvolvedMarkerView.road_type.in_([3, 4])) | ||
| .filter(InvolvedMarkerView.accident_severity.in_([1, 2])) |
There was a problem hiding this comment.
I know that they did it in their original SQL query - but over time we won't know what those number means, it would be best if they will be constants.
There was a problem hiding this comment.
this makes sense - do we have existing enums for any of our table values?
There was a problem hiding this comment.
Only some of them - you can see them in anyway\anyway\backend_constants.py
| @@ -87,7 +87,7 @@ jobs: | |||
| DOCKER_REPOSITORY_NGINX: ${{ env.SERVER }}/${{ env.DOCKER_REPOSITORY_NGINX }} | |||
| HASADNA_K8S_DEPLOY_KEY: ${{ secrets.HASADNA_K8S_DEPLOY_KEY }} | |||
| run: | | |||
| if [ "${GITHUB_REF}" == "refs/heads/master" ]; then | |||
There was a problem hiding this comment.
For example this code change doesn't belong this pull request
| COUNT(DISTINCT provider_and_id) num_of_accidents | ||
| FROM involved_markers_hebrew | ||
| WHERE road_type in (3,4) -- only inter-city | ||
| AND accident_severity in (1,2) GROUP BY 1,2 ORDER BY 3 DESC) as ACCIDENTS_COUNT''' |
There was a problem hiding this comment.
I do not understand the syntax :(, is this a docstring?, If yes, of the above or below
There was a problem hiding this comment.
No, A docstring is a string literal that occurs as the first statement in a module, function, class, or method definition.
For example:
def foo():
"""Return the number 1."""
return 1
This is the SQL query that should describe the function functionality - I agree with you that it's not very readable,
If you have any question about the query I can try to explain(I didn't write it so I am not fully sure about some of code in this query)
There was a problem hiding this comment.
The reason I left it there is that when I was trying to figure out how to implement it I had a hard time learning from existing code since I couldn't easily tell what the sqlalchemy code was achieving in sql vocabulary.
So I thought it may be useful to leave inside.
I can delete it if you don't see any values in it.
There was a problem hiding this comment.
Yes it will be better without it - we have the Pull request comments and the issue for this info
|
Opened a clean new PR |
closes #1579