ref(api): Split out "query" and "extensions" into separate data structures#425
ref(api): Split out "query" and "extensions" into separate data structures#425tkaemming wants to merge 4 commits into
Conversation
| body['project'] = [] | ||
| body['aggregations'] = [ | ||
| query, options = validate_request_content(body, schemas.SDK_STATS_QUERY_OPTIONS_SCHEMA, timer) | ||
| assert query['groupby'] == ['project_id'] # XXX: I hate this |
There was a problem hiding this comment.
I probably should have deleted this comment but it's true. I want to get rid of this.
| @split_query | ||
| def parse_and_run_query(dataset, validated_body, timer): | ||
| def parse_and_run_query(dataset, query, options, timer): | ||
| validated_body = {**query, **options} # XXX: compatibility |
There was a problem hiding this comment.
See note in the PR description about this. There are a lot of things downstream of this function that still rely on the query and options being in the same mapping and I don't want to tackle them all at once.
| options['project'] = list(set([event['project_id'] for event in result['data']])) | ||
|
|
||
| return query_func(dataset, {**body, 'conditions': conditions}, *args, **kwargs) | ||
| return query_func(dataset, query, options, *args, **kwargs) |
There was a problem hiding this comment.
The rest of the query (or body) was already being updated-in-place, so I made conditions do the same.
| col_exprs.extend([a[1] for a in query['aggregations']]) | ||
|
|
||
| # Return the set of all columns referenced in any expression | ||
| return set(chain(*[columns_in_expr(ex) for ex in col_exprs])) |
There was a problem hiding this comment.
This function is only used by places that can be restricted to the query structure and doesn't require any of the options data.
|
|
||
| if result['data']: | ||
| project_ids = list(set([event['project_id'] for event in result['data']])) | ||
| body['project_id'] = project_ids |
There was a problem hiding this comment.
This should have been project. I missed this during the initial review (I may have even suggested the wrong key. 😬 I feel like we can't get to type checks soon enough.)
tkaemming
changed the title
| def get_query_schema(self): | ||
| return EVENTS_QUERY_SCHEMA | ||
| def get_query_extensions_schema(self): | ||
| return EVENTS_QUERY_EXTENSIONS_SCHEMA |
There was a problem hiding this comment.
could you please provide an empty implementation for outcomes and groupedmessage so that they do not blow up as soon as we start querying them? Those dataset are already enabled even though none is querying them.
There was a problem hiding this comment.
This is already broken because the query execution right now has a hard dependency these fields (time series extension due to the query splitter and rate limiting.) I think we should fix that first so we're not splitting queries on columns that don't actually exist. If anybody was using them, I'd probably be a little more careful here, but I think they should be broken as-is because the underlying execution path makes assumptions about the data that aren't valid?
There was a problem hiding this comment.
"turbo" and "consistent" attributes should work as they are for both outcomes and groupedmessage. Actually I think they should work for all MergeTable datasets.
None is using them yet, but it will be a matter of days before we start at least for outcomes.
There was a problem hiding this comment.
I'm fine with moving both of those to general use extensions. I'd prefer in the future if those were opt-in on the dataset than inherited, though. I don't think we want to commit to every dataset having the same write path properties that allow us to use consistent.
This doesn't really change the fact though that these queries aren't going to work correctly until the splitter is modified, though.
There was a problem hiding this comment.
Ok, so here there are two separate problems (which do not seem depending on each other).
- time series issues during split. This is already broken now and it definitively doe not belong to this PR to fix it.
- consistent and turbo. These should still be extensions but should be used by our three datasets today. Those work properly today. Up to you if you want to add this support to groupedmessage and outcomes. Otherwise I can do it (ok, Manu probably will do that) when he starts querying outcomes. Both solutions are fine.
d7a8c92 to
6c5d71a
Compare
|
I'm going to spend some time investigating extensions before this is ready for another pass. |
|
Going to slightly change the approach here, and I think it'd be easier to start over with a clean review. |
2 participants
This is another step towards splitting out the generic query functionality and dataset-specific extensions.
This introduces the idea of a "composite schema" that merges N different schemata together — in our specific usage, this is the generic query schema, and any "extensions" that a dataset may use, such time series options. (I haven't split up the extensions into the
Datasetclass hierarchy yet, that is likely one of the next steps.) I see this as a short-term compatibility shim, so haven't invested too much into making it too user-friendly. Eventually I'd like to introduce a version 2 protocol that splits the schema into:{ "query": …, "extensions": …, # may contain another level for each "extension", e.g. {"timeseries": …, } }To limit the scope of this change, this is folded back into a
bodyvariable inparse_and_run_queryso that this change only really applies to the API endpoint and the query splitter. Later changes will address more of the tricky bits such ascolumn_expr,condition_expr, andraw_query.