-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(snuba-search): snuba group dataset search handle additional columns p2 #70812
feat(snuba-search): snuba group dataset search handle additional columns p2 #70812
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have to do this? will this affect the other discover search uses?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does discover not need this previously?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have to do this? looks weird
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.
@JoshFerge we have to deal with nested arrays
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.
got it, mind adding an example in the comment above?
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.
@JoshFerge added
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to do this timestamp business?
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.
@JoshFerge because we need to convert from a UNIX timestamp to datetime
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.
why? do we do this in our other query executors?
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.
@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( |
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.
nit: conditon
-> condition
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-release
has:x
server:
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.