-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(analytics-platform): Redesign sessions v3 for better partition pruning and indexing #39092
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
Conversation
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.
14 files reviewed, 6 comments
"toSecond": HogQLFunctionMeta("toSecond", 1, 1), | ||
"toUnixTimestamp": HogQLFunctionMeta("toUnixTimestamp", 1, 2), | ||
"toUnixTimestamp64Milli": HogQLFunctionMeta("toUnixTimestamp64Milli", 1, 1), | ||
"fromUnixTimestamp64Milli": HogQLFunctionMeta("fromUnixTimestamp64Milli", 1, 1), |
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.
style: Consider adding type signatures for this function like other similar functions (e.g., fromUnixTimestamp
on line 193) to ensure proper type checking and return type specification
"fromUnixTimestamp64Milli": HogQLFunctionMeta("fromUnixTimestamp64Milli", 1, 1), | |
"fromUnixTimestamp64Milli": HogQLFunctionMeta( | |
"fromUnixTimestamp64Milli", | |
1, | |
1, | |
signatures=[ | |
((IntegerType(),), DateTimeType()), | |
], | |
), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/hogql/functions/clickhouse/datetime.py
Line: 21:21
Comment:
**style:** Consider adding type signatures for this function like other similar functions (e.g., `fromUnixTimestamp` on line 193) to ensure proper type checking and return type specification
```suggestion
"fromUnixTimestamp64Milli": HogQLFunctionMeta(
"fromUnixTimestamp64Milli",
1,
1,
signatures=[
((IntegerType(),), DateTimeType()),
],
),
```
How can I resolve this? If you propose a fix, please make it concise.
posthog/hogql/database/schema/util/test/test_session_v3_where_clause_extractor.py
Show resolved
Hide resolved
def test_timestamp_unrelated_function_timestamp(self): | ||
actual = f( | ||
self.inliner.get_inner_where(parse("SELECT * FROM sessions WHERE like(toString(min_timestamp), 'b')")) | ||
) | ||
assert actual is None |
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.
style: Duplicate test method with identical logic to test_timestamp_unrelated_function above - consider removing or differentiating
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/hogql/database/schema/util/test/test_session_v3_where_clause_extractor.py
Line: 140:144
Comment:
**style:** Duplicate test method with identical logic to test_timestamp_unrelated_function above - consider removing or differentiating
How can I resolve this? If you propose a fix, please make it concise.
""" | ||
select | ||
session_id, | ||
from sessions |
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.
syntax: Extra comma creates syntax error in SQL
from sessions | |
session_id | |
from sessions |
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/hogql/database/schema/util/test/test_session_v3_where_clause_extractor.py
Line: 612:612
Comment:
**syntax:** Extra comma creates syntax error in SQL
```suggestion
session_id
from sessions
```
How can I resolve this? If you propose a fix, please make it concise.
select | ||
session.id as session_id, | ||
from events | ||
where session_id = {session_id} AND timestamp >= '1970-01-01' |
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.
syntax: Missing comma after session.id as session_id,
- this will cause a SQL syntax error
where session_id = {session_id} AND timestamp >= '1970-01-01' | |
select | |
session.id as session_id | |
from events | |
where session_id = {session_id} AND timestamp >= '1970-01-01' |
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/hogql/database/schema/test/test_sessions_v3.py
Line: 641:641
Comment:
**syntax:** Missing comma after `session.id as session_id,` - this will cause a SQL syntax error
```suggestion
select
session.id as session_id
from events
where session_id = {session_id} AND timestamp >= '1970-01-01'
```
How can I resolve this? If you propose a fix, please make it concise.
e7216cb
to
2f78f49
Compare
|
||
if node.op == CompareOperationOp.Eq: | ||
if is_left_constant and is_session_id_string_expr(node.right, self.context): | ||
left_timestamp_expr = self.session_id_str_to_timestamp_expr(node.left) |
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.
It is possible for session_id_str_to_timestamp_expr
to return None? If so, how will be known about it?
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.
session_id_str_to_timestamp_expr
can be null (it uses accurateCastOrNull(x, 'UUID')
behind the scenes)
I don't believe that this being nullable has performance implications, and in testing, if I use a garbage constant string for the UUID (e.g. select * from sessions where session_id = 'garbage'
), clickhouse was smart enough to know it doesn't need to load any parts
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.
Took my time, but really didn't spot anything
posthog/hogql/database/schema/util/test/test_session_v3_where_clause_extractor.py
Outdated
Show resolved
Hide resolved
6b51daa
to
0dd5e6d
Compare
0dd5e6d
to
918a13a
Compare
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.
🚢
Problem
I found that we were not making good use of indexing and partition pruning, this PR fixes that. See some discussion #38809 (comment)
Changes
How did you test this code?
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Changelog: (features only) Is this feature complete?