-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
added requestcounts endpoint #393
Merged
Merged
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
from sqlalchemy import func | ||
from .dataService import DataService | ||
from .databaseOrm import Ingest as Request | ||
|
||
|
||
class RequestCountsService(object): | ||
def __init__(self, config=None, tableName="ingest_staging_table"): | ||
self.dataAccess = DataService(config, tableName) | ||
|
||
async def get_req_counts(self, | ||
startDate=None, | ||
endDate=None, | ||
ncList=[], | ||
requestTypes=[], | ||
countFields=[]): | ||
""" | ||
For each countField, returns the counts of each distinct value | ||
in that field, given times, ncs, and request filters. | ||
E.g. if countsFields is ['requesttype', 'requestsource'], returns: | ||
{ | ||
'lastPulled': 'Timestamp', | ||
'data': [ | ||
{ | ||
'field': 'requesttype', | ||
'counts': { | ||
'Graffiti Removal': 'Int', | ||
'Bulky Items': 'Int', | ||
... | ||
} | ||
}, | ||
{ | ||
'field': 'requestsource', | ||
'counts': { | ||
'Mobile App': 'Int', | ||
'Driver Self Report': 'Int', | ||
... | ||
} | ||
} | ||
] | ||
} | ||
""" | ||
|
||
# filter by date, nc, and requestType (if provided) | ||
filters = [ | ||
Request.createddate > startDate if startDate else True, | ||
Request.createddate < endDate if endDate else True, | ||
Request.ncname.in_(ncList) if ncList else True, | ||
Request.requesttype.in_(requestTypes) if requestTypes else True | ||
] | ||
|
||
data = [] | ||
for field in countFields: | ||
# make sure the field exists in the Request model | ||
if not getattr(Request, field, None): | ||
continue | ||
|
||
# run count/groupby query | ||
results = self.dataAccess.session \ | ||
.query(field, func.count()) \ | ||
.filter(*filters) \ | ||
.group_by(field) \ | ||
.all() | ||
|
||
# add results to data set | ||
data.append({ | ||
'field': field, | ||
'counts': dict(results) | ||
}) | ||
|
||
return DataService.withMeta(data) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im thinking that we might want to adjust this to hit data service with countFields and filters and create an aggregateQuery method in dataservice....so functionality will look like
select [countFields] from table where [filters]
Then do a pandas groupby(countFields).size()
Should follow something like this https://stackoverflow.com/a/38933130
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason behind this is to keep the database logic separate from any of the services which will make testing/validation much easier(when we get around to it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes sense, I can move the database logic into the dataservice. We might want to get away from constructing SQL queries by string concatenation though, it's kind of error-prone and hard to read. The ORM in sqlalchemy basically generates (and executes) SQL queries in a cleaner way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i agree, we can def tweak the existing query method to use the orm instead of creating a 💩 query string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #398 in the meantime so whoever takes it can use your code as an example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok cool, yeah I'll take that one. I can work on it while I'm creating the aggregationQuery in the dataservice. I like the aggregationQuery concept, could be useful for other endpoints down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!!
Im hoping that with query and aggregationQuery itll keep the backend to easy categorization of services. Either getting a single thing, a lot of stuff, or a summary of groupings
That way there wont be weird complex querying per endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I gave the aggregationQuery a shot, also removed SQL string concatenation and used ORM instead in the pins service and request detail service.