feat(snuba-search): snuba group dataset search handle additional columns p2 #70812
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #70812 +/- ##
===========================================
+ Coverage 50.46% 80.04% +29.58%
===========================================
Files 6469 6509 +40
Lines 289328 290807 +1479
Branches 49888 50126 +238
===========================================
+ Hits 146003 232772 +86769
+ Misses 142892 57601 -85291
- Partials 433 434 +1
|
| @@ -1322,6 +1322,9 @@ def default_filter_converter( | |||
| 1, | |||
| ) | |||
| else: | |||
| # pull out the aliased expression if it exists | |||
| if isinstance(lhs, AliasedExpression): | |||
There was a problem hiding this comment.
why do we have to do this? will this affect the other discover search uses?
There was a problem hiding this comment.
@JoshFerge it would just error out. I am pretty certain it doesn't impact discover or else we'd see the error about AliasedExpression
There was a problem hiding this comment.
why does discover not need this previously?
There was a problem hiding this comment.
@JoshFerge It only fails on the project_id input that's used as a filter. It seems like we only use this function default_filter_converter on small subset of filter types currently even though it's written to accommodate a lot more. I don't really know the history here and the code is convoluted. My guess is this function used to be used for more things but at some point only got used for a subset of possible filters.
There was a problem hiding this comment.
got it. mind just adding more info to that comment then?
src/sentry/search/snuba/executors.py
Outdated
| raw_column = map_field_name_from_format_search_filter(lhs[1][0]) | ||
| rhs = [query_builder.resolve_column(raw_column)] | ||
| if len(lhs[1]) > 1: | ||
| rhs.append(lhs[1][1]) | ||
| lhs = Function(lhs[0], rhs) |
There was a problem hiding this comment.
why do we have to do this? looks weird
There was a problem hiding this comment.
@JoshFerge we have to deal with nested arrays
There was a problem hiding this comment.
got it, mind adding an example in the comment above?
src/sentry/search/snuba/executors.py
Outdated
| raw_column = map_field_name_from_format_search_filter(lhs) | ||
| lhs = query_builder.resolve_column(raw_column) | ||
| else: | ||
| # need to convert dates to timestamps |
There was a problem hiding this comment.
why do we need to do this timestamp business?
There was a problem hiding this comment.
@JoshFerge because we need to convert from a UNIX timestamp to datetime
There was a problem hiding this comment.
why? do we do this in our other query executors?
There was a problem hiding this comment.
@JoshFerge I realize now that we already handle the date filter with calculate_start_end so I can remove that special logic, we just skip that filter
src/sentry/search/snuba/executors.py
Outdated
| self.get_basic_event_snuba_condition( | ||
| search_filter, joined_entity, organization.id, project_ids, environments | ||
| ) | ||
| conditon = self.get_basic_event_snuba_condition( |
scefali
deleted the
feat/snuba-group-dataset-search-handle-additional-columns-p2
branch
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This PR handles additional filters for the snuba-heavy search:
issue:project:date:first-releasehas:xserver:I'm attempting to use as many pieces from existing search code as possible which is why I'm leveraging the discover query builder as well as
format_search_filter. It's kind of a Frankenstein monster of composed parts, however.