chore: trends query runner tests #18479
Conversation
|
Hey @Gilbert09! 👋 |
Gilbert09
force-pushed
the
tom/trends-tests
branch
from
a66120f to
f8940b7
Compare
Gilbert09
force-pushed
the
tom/trends-old-tests
branch
from
76986a9 to
cafd665
Compare
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Co-authored-by: Marius Andra <[email protected]>
|
Hey @Gilbert09! 👋 |
| if ContainsLazyJoinType(expression).contains_lazy_join: | ||
| return False |
| """ | ||
| ) | ||
|
|
||
| compare_operators = self._compare_operators(query, "e") | ||
|
|
||
| assert len(compare_operators) == 1 | ||
| assert compare_operators[0] == ast.CompareOperation( | ||
| left=ast.Field(chain=["event"]), | ||
| left=ast.Field(chain=["team_id"]), |
There was a problem hiding this comment.
How come you're comparing the team_id here? This field is something that gets handled outside of HogQL... so these queries would have it double. It's in many ways reserved in HogQL (e.g. you can't make an alias with this name), so seeing it in tests makes me automatically wonder if something is wrong and if we're leaking data... 🤔
There was a problem hiding this comment.
Because we not intentionally exclude event from the where clause, so just wanted to pick any other field for the tests instead. But I get it being possibly weird seeing this in a test
| parse_expr( | ||
| "timestamp >= {date_from} - {inclusive_lookback}", | ||
| placeholders={ | ||
| **self.query_date_range.to_placeholders(), | ||
| **self._interval_placeholders(), | ||
| }, | ||
| ), | ||
| parse_expr( | ||
| "timestamp <= {date_to}", | ||
| placeholders={ | ||
| **self.query_date_range.to_placeholders(), | ||
| **self._interval_placeholders(), | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Both ends of the timestamp are >= and <=. Usually I'd expect to see on bounded, and one unbounded. Bug or on purpose?
There was a problem hiding this comment.
Great question, lemme investigate
There was a problem hiding this comment.
Checked, it's intentional
| filters.extend( | ||
| [ | ||
| parse_expr( | ||
| "timestamp >= {date_from_with_adjusted_start_of_interval}", | ||
| placeholders=self.query_date_range.to_placeholders(), | ||
| ), | ||
| parse_expr( | ||
| "timestamp <= {date_to}", | ||
| placeholders=self.query_date_range.to_placeholders(), | ||
| ), |
There was a problem hiding this comment.
Same question with the dateranges here
| # dateRange=DateRange( | ||
| # date_from=filter.date_from.isoformat() if filter.date_from is not None else "all", | ||
| # date_to=filter.date_to.isoformat() if filter.date_to is not None else None, | ||
| # ), |
| self.assertDictContainsSubset({"breakdown_value": "person1", "aggregated_value": 1}, event_response[0]) | ||
| self.assertDictContainsSubset({"breakdown_value": "person2", "aggregated_value": 1}, event_response[1]) | ||
|
|
||
| # TODO: test_account_filters conversion |
There was a problem hiding this comment.
What's to convert with them?
There was a problem hiding this comment.
Old comment. Lemme uncomment and figure that out. Will merge this now and get these fixes out next
Problem
Changes
How did you test this code?