ref(query): Isolate query formatting from the remainder of query processing by tkaemming · Pull Request #418 · getsentry/snuba

tkaemming · 2019-08-15T00:41:24Z

This change isolates the query formatting concerns from parse_and_run_query.

tkaemming · 2019-08-15T00:44:55Z

    )


+def format_query(dataset, body, table, prewhere_conditions, final) -> str:


My goal is to eventually replace the body, table, prewhere_conditions, final part of this signature with a Query construct that is separate from the request body format. This will require (probably substantial) changes to column_expr and condition_expr.

any chance we could have type hints in the signature (together with the result). It would make it easier to understand the method body.

This seems so much more readable. One question at your plan for the Query construct. Are you going to create a class that takes body, dataset, table, pre_where and that has a method that returns the query string?
Second, how will this play with multiple dataset? Is every dataset going to provide its Query construct to allow for dataset specific logic in query parsing? Assuming we will allow datasets to provide their own logic.

any chance we could have type hints in the signature (together with the result). It would make it easier to understand the method body.

I can add them for dataset, table, and final, but the ones for body are going to take more work. I experimented a bit with this last week — anything that ends up using column_expr or condition_expr is going to take a lot of work to nail down. We can start partially typing Query into a TypedDict if you think that's helpful. I think we can at least have a comprehensive set of valid keys, and types that are more specific than Any for a handful of them (but unfortunately not really any of those that would be useful.)

yeah, even Any would be reasonable for body, so that it sets expectations.

This seems so much more readable. One question at your plan for the Query construct. Are you going to create a class that takes body, dataset, table, pre_where and that has a method that returns the query string?
Second, how will this play with multiple dataset? Is every dataset going to provide its Query construct to allow for dataset specific logic in query parsing? Assuming we will allow datasets to provide their own logic.

I think this is probably a little bit clearer when considered in conjunction with #425, but my general thinking is:

The API endpoint turns a request body into a Request object (might be a better name for that) that includes query (generic query schema, this will be the exact same for all datasets) data and extension (dataset-specific schema/schemata) data.

The Request object is provided to the dataset, which can mutate the query or do additional validation based on the extensions. This preferably returns just the Query but might need to return the full Request, at least for now. (More on this below.)

The query is evaluated (probably the API responsibility, but not 100% sure about this yet), which does the following in this order (the order here is important, since it's not the exact same order as raw_query is today):

Rate limit accounting. This will probably require access to attributes in the Request (specifically project.) Trying to extract them from the Query seems like a lot of work for little benefit.

Caching, and (optionally) deduplication. These two processes are pretty tightly coupled in right now in practical usage but are implemented independently. I think it makes sense to make that coupling explicit. Right now you can have deduplication without caching which makes very little practical sense.

Query splitting. This will only be enabled for some datasets. This may or may not require access to the Request data. The current implementation does. Not quite sure how I see this evolving yet. I think this makes more sense to happen after caching and deduplication than the way it's implemented today, where we cache each intermediate execution step.

Query formatting. (I think there's an argument to push some of this down into the Reader, I'm not set one way or another yet.)

Query execution. Pretty much just what the Reader does now.

Serialize and return the response, etc.

cool. thanks

tkaemming · 2019-08-15T00:45:48Z

+    body['conditions'].extend([
        ('timestamp', '>=', from_date),
        ('timestamp', '<', to_date),
    ])


This will eventually move to the time series dataset.

tkaemming · 2019-08-15T00:45:57Z

-    select_clause = u'SELECT {}'.format(', '.join(select_exprs))
-
-    from_clause = u'FROM {}'.format(table)
+        body['conditions'].append(('project_id', 'IN', project_ids))


This will eventually move to the events dataset.

I suspect you may run into problems if you update the body of the query during split queries since the body is passed down by the split_query wrapper to "parse_and_run_query".
Same above at line 251. Could you please verify these mutations are harmless. Though I think the processing would be more robust if we treated body as immutable.

There's no concerns around mutating the body here since this is already deepcopied on line 233.

This was also already mutating the conditions list (specifically, the copy of the original conditions list from line 233) since the conditions list is created (as a default) during schema validation:

snuba/snuba/schemas.py

Lines 92 to 104 in 2d55ce8

'conditions': {

'type': 'array',

'items': {

'anyOf': [

{'$ref': '#/definitions/condition'},

{

'type': 'array',

'items': {'$ref': '#/definitions/condition'},

},

],

},

'default': [],

},

The branch to set default or return a default value should never be set, other than potentially during tests.

I agree that we should make this immutable moving forward. I think that's part of a larger change, though. The design of the splitter will have to change, and even things like alias_expr mutate the body:

snuba/snuba/util.py

Line 187 in 2d55ce8

alias_cache = body.setdefault('alias_cache', [])

ok, I did not see line 233. If we are not introducing a mutation here. then it is fine

Yeah, I think the fact that it's not obvious is a smell in and of itself. I think there are two problems here:

If splitter is the component that is concerned with the mutations to the query, it should be the one doing the copy before it yields control to other parts of the codebase, not relying on them to act in its best interests. The way the code is structured right now makes that really easy to get wrong, and I think your concern around it is totally justified.

In general the splitter is implemented in a weird part of the query execution pipeline today. If splitting occurred after condition rewriting and things (as noted above), it wouldn't be an issue. The splitter would probably still have to make copies of the original payload, but it'd probably be more obvious why.

tkaemming · 2019-08-15T00:46:25Z

    else:
        final = False
+        if 'sample' not in body:
+            body['sample'] = settings.TURBO_SAMPLE_RATE


This will eventually move to the events dataset.

tkaemming · 2019-08-15T00:47:04Z

-        joins.append(u'ARRAY JOIN {}'.format(body['arrayjoin']))
-    join_clause = ' '.join(joins)
+    if 'sample' not in body and config_sample is not None:
+        body['sample'] = config_sample


I'm not sure how helpful this configuration parameter is, since it changes the response so drastically. I'd like to remove it but this preserves the existing behavior for now.

tkaemming · 2019-08-15T00:47:37Z

-    prewhere_clause = ''
-    if prewhere_conditions:
-        prewhere_clause = u'PREWHERE {}'.format(util.conditions_expr(dataset, prewhere_conditions, body))
+            body['conditions'] = list(filter(lambda cond: cond not in prewhere_conditions, body['conditions']))


This will probably stay shared but PREWHERE_KEYS will become a dataset configuration.

tkaemming · 2019-08-15T00:48:19Z

        'num_projects': len(project_ids),
-        'sample': sample,
+        'sample': body.get('sample'),
    }


This is really just a grab bag of random attributes at the moment, we should figure out a more reasonable thing to do here.

tkaemming · 2019-08-15T00:49:00Z

+
+    limitby_clause = ''
+    if 'limitby' in body:
+        limitby_clause = 'LIMIT {} BY {}'.format(*body['limitby'])


This was moved, but I'm pretty sure this won't actually work because it's not using column_expr. I didn't test it.

moved from where ?

snuba/snuba/api.py

Lines 370 to 372 in 2d55ce8

limitby_clause = ''

if 'limitby' in body:

limitby_clause = 'LIMIT {} BY {}'.format(*body['limitby'])

Almost everything here should be a move out of parse_and_run_query. Sorry if this wasn't clear from the description.

fpacifici

Could you please verify we won't have problems with split queries if we make changes to the body of the query (or better keep body immutable - not providing this guarantee will be a little weak when we will allow the datasets to do their own optimizations and splitting).

fpacifici · 2019-08-19T17:06:00Z

    )


+def format_query(dataset, body, table, prewhere_conditions, final) -> str:


any chance we could have type hints in the signature (together with the result). It would make it easier to understand the method body.

fpacifici · 2019-08-19T17:20:20Z

    )


+def format_query(dataset, body, table, prewhere_conditions, final) -> str:


This seems so much more readable. One question at your plan for the Query construct. Are you going to create a class that takes body, dataset, table, pre_where and that has a method that returns the query string?
Second, how will this play with multiple dataset? Is every dataset going to provide its Query construct to allow for dataset specific logic in query parsing? Assuming we will allow datasets to provide their own logic.

fpacifici · 2019-08-19T17:20:57Z


+def format_query(dataset, body, table, prewhere_conditions, final) -> str:
+    """Generate a SQL string from the parameters."""
+    aggregate_exprs = [util.column_expr(dataset, col, body, alias, agg) for (agg, col, alias) in body['aggregations']]


@markstory you have a PR that changes this. You probably want to keep an eye on this one.

fpacifici · 2019-08-19T17:24:49Z

+
+    join_clause = ''
+    if 'arrayjoin' in body:
+        join_clause = u'ARRAY JOIN {}'.format(body['arrayjoin'])


do we have to "unicodify" this one ?

I'm not sure what this question means. I don't think we need to do any specific encoding here or anything. This will be a unicode string. The u prefix is actually unnecessary for both of these strings due to Python 3. Behaviorally, this is really just the same as before, with the exception that this removes some unnecessary list operations that weren't valid per the JSON schema.

I was talking about body['arrayjoin']. But it seems it is ok.

fpacifici · 2019-08-19T17:31:44Z

+
+    limitby_clause = ''
+    if 'limitby' in body:
+        limitby_clause = 'LIMIT {} BY {}'.format(*body['limitby'])


moved from where ?

fpacifici · 2019-08-19T17:42:08Z

-    select_clause = u'SELECT {}'.format(', '.join(select_exprs))
-
-    from_clause = u'FROM {}'.format(table)
+        body['conditions'].append(('project_id', 'IN', project_ids))


I suspect you may run into problems if you update the body of the query during split queries since the body is passed down by the split_query wrapper to "parse_and_run_query".
Same above at line 251. Could you please verify these mutations are harmless. Though I think the processing would be more robust if we treated body as immutable.

fpacifici

The typing on the format method are not a blocker. Still welcome if you want to add it before committing.

fpacifici · 2019-08-19T22:59:04Z

    )


+def format_query(dataset, body, table, prewhere_conditions, final) -> str:


cool. thanks

fpacifici · 2019-08-19T22:59:48Z

+
+    join_clause = ''
+    if 'arrayjoin' in body:
+        join_clause = u'ARRAY JOIN {}'.format(body['arrayjoin'])


I was talking about body['arrayjoin']. But it seems it is ok.

fpacifici · 2019-08-19T23:01:22Z

-    select_clause = u'SELECT {}'.format(', '.join(select_exprs))
-
-    from_clause = u'FROM {}'.format(table)
+        body['conditions'].append(('project_id', 'IN', project_ids))


ok, I did not see line 233. If we are not introducing a mutation here. then it is fine

tkaemming

The typing on the format method are not a blocker. Still welcome if you want to add it before committing.

I think it's a good idea and I had it partially started on another branch, I'll bring over what I can.

tkaemming · 2019-08-19T23:54:33Z

-    select_clause = u'SELECT {}'.format(', '.join(select_exprs))
-
-    from_clause = u'FROM {}'.format(table)
+        body['conditions'].append(('project_id', 'IN', project_ids))


Yeah, I think the fact that it's not obvious is a smell in and of itself. I think there are two problems here:

If splitter is the component that is concerned with the mutations to the query, it should be the one doing the copy before it yields control to other parts of the codebase, not relying on them to act in its best interests. The way the code is structured right now makes that really easy to get wrong, and I think your concern around it is totally justified.

In general the splitter is implemented in a weird part of the query execution pipeline today. If splitting occurred after condition rewriting and things (as noted above), it wouldn't be an issue. The splitter would probably still have to make copies of the original payload, but it'd probably be more obvious why.

tkaemming · 2019-08-20T00:00:59Z

I think it's a good idea and I had it partially started on another branch, I'll bring over what I can.

Actually, I'm going to do both of these (splitter changes and type addition) in two follow ups after this because I want to move format_query out of snuba/api.py and into snuba/query.py and that seems like a better time to introduce it rather than having to immediately move all the typing imports over as well.

## Bump sentry-conventions to 0.11.0 Updates `sentry-conventions` from 0.10.0 to [0.11.0](https://github.com/getsentry/sentry-conventions/releases/tag/0.11.0). ### Changes in 0.11.0 **New Features** - Add `session.id` attribute (#412) - Update several attributes to latest OTel representation (#418) - Add `db.operation.batch.size` (#407) - Add `app.vitals.start.prewarmed` attribute (#379) - Add and deprecate runtime context attributes (#383) - Add Android Runtime (ART) GC and memory attributes (#382) - Add `db.stored_procedure.name` and ensure span name attribute consistency (#398) - Add Cloudflare SDK attributes (#392) - Add missing GCP and FaaS attributes (#403) **Bug Fixes** - Set `pii: 'maybe'` on `faas` string attributes (#415) - Add missing Cloudflare visibility attributes (#408) **Internal** - Remove `sdks` field from attribute schema and definitions (#410)

ref(query): Split out query formatting from rewriting

c293528

tkaemming requested a review from a team August 15, 2019 00:41

tkaemming commented Aug 15, 2019

View reviewed changes

fpacifici requested changes Aug 19, 2019

View reviewed changes

fpacifici approved these changes Aug 19, 2019

View reviewed changes

tkaemming commented Aug 19, 2019

View reviewed changes

Merge branch 'master' into query-refactoring

b9dd99b

tkaemming merged commit de26a44 into master Aug 20, 2019

tkaemming deleted the query-refactoring branch August 20, 2019 00:07

constantinius mentioned this pull request Jun 10, 2026

chore(deps): Bump sentry-conventions to 0.11.0 #8013

Merged

		)


		def format_query(dataset, body, table, prewhere_conditions, final) -> str:

	'conditions': {
	'type': 'array',
	'items': {
	'anyOf': [
	{'$ref': '#/definitions/condition'},
	{
	'type': 'array',
	'items': {'$ref': '#/definitions/condition'},
	},
	],
	},
	'default': [],
	},

	limitby_clause = ''
	if 'limitby' in body:
	limitby_clause = 'LIMIT {} BY {}'.format(*body['limitby'])

Uh oh!

Uh oh!

Conversation

tkaemming commented Aug 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fpacifici left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fpacifici left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkaemming left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkaemming commented Aug 20, 2019