feat(devexp): add ability to parse simple storage queries #5776

volokluev · 2024-04-15T19:59:27Z

This is the most simple version of this PR:

Work that is needed to make this really work:

Add required_time_column to storages so we can do the date alignment logic we use for entities
- or we just don't care like what we're doing right now
Add BasicFunctionsProcessor as a post_process step so it's entity independent

…the parser

sentry-io · 2024-04-15T19:59:35Z

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: snuba/query/snql/parser.py

Function	Unhandled Issue
`parse_snql_query`	PostProcessingError: None snql_dataset_query_view... `Event Count:` 4

_{Did you find this useful? React with a 👍 or 👎}

codecov · 2024-04-15T20:19:25Z

Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time.

❌ Failed Test Results:

Completed 1 tests with 1 failed, 0 passed and 0 skipped.

View the full list of failed tests

Test Description	Failure message
Testsuite: pytest Test name: ::tests.query.snql.test_storage_query Envs: - default	No failure message available

… make tag queries work tho

…parse_storage

…dation PR to come in

…parse_storage

volokluev · 2024-04-30T22:59:22Z

snuba/query/dsl.py

@@ -23,18 +47,6 @@ def literal(value: OptionalScalarType, alias: str | None = None) -> Literal:
 return Literal(alias, value)


-def snuba_tags_raw(indexer_mapping: int) -> SubscriptableReference:


was not used

volokluev · 2024-04-30T22:59:56Z

snuba/query/dsl.py

+def equals(
+ lhs: Expression | OptionalScalarType, rhs: Expression | OptionalScalarType
+) -> FunctionCall:


happy to change this but I found it useful to not have to specify literals

volokluev · 2024-04-30T23:00:39Z

snuba/query/logical.py

@@ -81,3 +79,73 @@ def _transform_expressions_impl(

 def _transform_impl(self, visitor: ExpressionVisitor[Expression]) -> None:
 pass
+
+
+class StorageQuery(ProcessableQuery[Storage]):


TODO: I think this type can be generalized

Why did you decide to parse into a StorageQuery rather than a ClickhouseQuery?

Good question. The main thing is that the Table datasaoure does not resemble the entity in any real way and it's only by the time we get to the StorageProcessing stage that we can process the clickhouse query.

Ideally what I would do is remove Table as a datasource and probably remove ClickhouseQuery later.

Why does it matter that Table doesn't resemble Entity?

volokluev · 2024-04-30T23:10:38Z

snuba/query/snql/parser.py

@@ -1532,8 +1617,6 @@ def parse_snql_query(
 with sentry_sdk.start_span(op="processor", description="time_based_processing"):
 _post_process(query, [_replace_time_condition], settings)

- # XXX: Select the entity to be used for the query. This step is temporary. Eventually


RIP this comment

'working'

kylemumma

My biggest concern would be the introduction of a new StorageQuery class rather than going directly to a ClickhouseQuery.

kylemumma · 2024-05-02T14:31:22Z

snuba/query/logical.py

@@ -81,3 +79,73 @@ def _transform_expressions_impl(

 def _transform_impl(self, visitor: ExpressionVisitor[Expression]) -> None:
 pass
+
+
+class StorageQuery(ProcessableQuery[Storage]):


Why did you decide to parse into a StorageQuery rather than a ClickhouseQuery?

kylemumma · 2024-05-02T14:32:02Z

snuba/query/dsl.py

+def equals(
+ lhs: Expression | OptionalScalarType, rhs: Expression | OptionalScalarType
+) -> FunctionCall:


kylemumma · 2024-05-02T14:55:47Z

tests/query/snql/test_storage_query.py

+
+
+@pytest.mark.parametrize("query_body, expected_query", test_cases)
+def test_format_expressions(query_body: str, expected_query: StorageQuery) -> None:


nit: im not sure if this is what you meant to name it but i was expecting something more like test_parse_storage or something

kylemumma · 2024-05-02T15:07:43Z

snuba/query/snql/parser.py

+ try:
+ return StorageKey(node.text)
+ except Exception:
+ raise ParsingException(f"{node.text} is not a valid Storage name")


unrelated to current PR, but one thought I have in general is that it might be nice to be able to parse StorageKey and EntityKey even if it doesnt exist. And then later on you get an error if you try to access one that doesnt exist. This would make using fake ones in testing easier.

kylemumma · 2024-05-02T15:09:31Z

snuba/query/snql/parser.py


- assert isinstance(match, QueryEntity) # mypy
+ assert isinstance(match, (QueryEntity, QueryStorage)) # mypy


random comment that you can ignore: if you care to read a relevant excerpt of the pragmatic programmer about "assertive programming" https://www.informit.com/articles/article.aspx?p=2982114&seqNum=3

kylemumma · 2024-05-02T15:12:56Z

snuba/query/snql/parser.py

+ if not isinstance(match, (QueryEntity, QueryStorage)):
+ import pdb
+
+ pdb.set_trace()
+ print(match)


Suggested change

if not isinstance(match, (QueryEntity, QueryStorage)):

import pdb

pdb.set_trace()

print(match)

kylemumma · 2024-05-02T15:31:35Z

snuba/query/snql/parser.py

 query.transform_expressions(mangle_column_value)
+ print(query)


Suggested change

print(query)

volokluev added 9 commits April 12, 2024 10:23

logical query can now include a storage

7633006

working storage query

2689bb2

remove unused imports

78db72a

working test for nested fields

024875f

test composite query doesn't work because we do entity validation in …

bb3c8c3

…the parser

cleanup composite stuff

60e1295

remove print statements

c72dd2e

rename

3c0598a

unnecessary comment

5f5c346

volokluev added 8 commits April 15, 2024 16:34

fix test

c40becd

fix the mypy issues??

5cda379

working end to end storage query for metrics_summaries, still need to…

edc5448

… make tag queries work tho

Fix mypy although not too happy with mql parser changes

1bc0459

Merge branch 'master' of github.com:getsentry/snuba into volo/devexp/…

e4c59da

…parse_storage

kind of fix up composite query parsing, still waiting for entity vali…

64cf7d1

…dation PR to come in

Merge branch 'master' of github.com:getsentry/snuba into volo/devexp/…

9349313

…parse_storage

fix mypy again?

559e698

volokluev commented Apr 30, 2024

View reviewed changes

volokluev added 2 commits April 30, 2024 16:04

remove pdb

422b133

remove unused code

56d1148

volokluev commented Apr 30, 2024

View reviewed changes

volokluev added 5 commits April 30, 2024 16:21

wip, composite query test still not working

477348c

wip, aliases not working, possible bug

975c298

doesn't work still, need to checkout master and see if I can reproduce

81aa0dc

get the composite test

5120b4e

'working'

remove erroneous test

6deda59

kylemumma reviewed May 2, 2024

View reviewed changes

volokluev closed this May 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(devexp): add ability to parse simple storage queries #5776

feat(devexp): add ability to parse simple storage queries #5776

volokluev commented Apr 15, 2024 •

edited

sentry-io bot commented Apr 15, 2024

codecov bot commented Apr 15, 2024 •

edited

volokluev Apr 30, 2024

volokluev Apr 30, 2024

kylemumma May 2, 2024

volokluev Apr 30, 2024

kylemumma May 2, 2024

volokluev May 2, 2024

kylemumma May 3, 2024

volokluev Apr 30, 2024

kylemumma left a comment

kylemumma May 2, 2024

kylemumma May 2, 2024

kylemumma May 2, 2024

kylemumma May 2, 2024

kylemumma May 2, 2024

kylemumma May 2, 2024

kylemumma May 2, 2024

		@@ -23,18 +47,6 @@ def literal(value: OptionalScalarType, alias: str \| None = None) -> Literal:
		return Literal(alias, value)


		def snuba_tags_raw(indexer_mapping: int) -> SubscriptableReference:



		@pytest.mark.parametrize("query_body, expected_query", test_cases)
		def test_format_expressions(query_body: str, expected_query: StorageQuery) -> None:


		assert isinstance(match, QueryEntity) # mypy
		assert isinstance(match, (QueryEntity, QueryStorage)) # mypy

		query.transform_expressions(mangle_column_value)
		print(query)

feat(devexp): add ability to parse simple storage queries #5776

feat(devexp): add ability to parse simple storage queries #5776

Conversation

volokluev commented Apr 15, 2024 • edited

sentry-io bot commented Apr 15, 2024

🔍 Existing Issues For Review

codecov bot commented Apr 15, 2024 • edited

❌ Failed Test Results:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylemumma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volokluev commented Apr 15, 2024 •

edited

codecov bot commented Apr 15, 2024 •

edited