fix: more tz/date fiddling to get this property thing working #27423
Conversation
| -- but we do care about whether timezones have an odd impact | ||
| -- so, we extend the range by a day on each side so that timezones don't cause issues | ||
| WHERE timestamp > ${dayjs(earliestTimestamp).subtract(1, 'day')} | ||
| AND timestamp < ${dayjs(latestTimestamp).add(1, 'day')} |
There was a problem hiding this comment.
What about utc + formatting, won't this return a dayjs object, or is its default string representation the correct format?
There was a problem hiding this comment.
answered here 😊 https://github.com/PostHog/posthog/pull/27423/files#r1909583592
| -- we don't really care about the absolute value, | ||
| -- but we do care about whether timezones have an odd impact | ||
| -- so, we extend the range by a day on each side so that timezones don't cause issues | ||
| WHERE timestamp > ${dayjs(earliestTimestamp).subtract(1, 'day')} |
There was a problem hiding this comment.
@rafaeelaudibert always best to comment on a file or line so we can thread 😊
Don't you need a .format() call to avoid the problem we had previously?
I tried with and without and I don't believe it affects the HogQL query output
We don't want to pass in the number value earliesTimestamp since then CH doesn't use the index
But passing in dayjs object constructed from it seems to get to the same place as formatting ourselves
There was a problem hiding this comment.
Got it, probably because that's the default implementation for toString(). I'd still prefer the format call because I can understand what's actually interpolated. If I look at the code right now I have no idea if we're sending an UNIX timestamp, a UTC string, some ISO string, etc.
It's not blocking, though :)
There was a problem hiding this comment.
pulling into this tread in one place with @joshsny too 😊
with this dayjs alone I get
greater(toTimeZone(events.timestamp, 'UTC'), '2025-01-08 09:57:56')
if I do dayjs(earliestTimestamp).subtract(1, 'day').utc()
then I get greater(toTimeZone(events.timestamp, 'UTC'), '2025-01-08 09:57:56')
and if I do the format on top then ofc I get whatever I formatted
the value is already UTC... the problem actually comes because HogQL is wrapping the column in the project TZ when I would love to say... this is all under my control and already UTC
I think I prefer passing in the dayjs object to the hogql string cos then we could fix things in one place in that template for all queries that pass a dayjs whereas if we pass strings then we always have to edit everywhere we pass the strings in
but 🤷 it's late and i just want to fix for users for now 🙈 🤣
There was a problem hiding this comment.
I think I prefer passing in the dayjs object to the hogql string cos then we could fix things in one place in that template for all queries that pass a dayjs whereas if we pass strings then we always have to edit everywhere we pass the strings in
That's a great argument, and I've actually learned something. We're doing that already, that's why you're getting the same thing
There was a problem hiding this comment.
finally found something confusing about timezones 🫠
|
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
* master: (659 commits) fix: more tz/date fiddling to get this property thing working (#27423) fix(hogql): support virtual tables with lazy tables (#27404) fix: Removing target options for survey resets its value to remove validation error (#27139) chore: Use `actions/{download,upload}-artifact@v4` (#27413) feat(clickhouse): when HTTP pass ca_cert and verify to pool manager (#27399) fix: typo in dashboard template configuration (#27417) fix(experiments): Force refresh when `start_date` is provided (#27396) feat(data-warehouse): Reset the pipeline source files when resync is selected on the frontend (#27402) chore(data-warehouse): Added SSL error as a non retryable error (#27395) chore(data-warehouse): Upgrade deltalake package (#27393) chore(data-warehouse): Kill the delta subprocess if need be (#27392) feat: Add P75 quantile (#27409) fix: property labels need to be known about in the backend but are defined in the front end (#27328) fix(editor-3001): show header on mobile for editor (#27373) chore: Enable web vitals capture (#27394) feat: Add human friendly comparison periods toggle (#27176) feat: promote data warehouse from taxonomic filter component (#27364) chore(experiments): Improve result state resetting (#27391) fix: Track records completed in heartbeat (#26686) fix(data-warehouse): Handle NaN values from SQL sources (#27360) ...
follow-up to #27386
in that PR we were right that the formatting was incorrect, but we unfortunately had an example that worked anyway after the fix. but property loading generally still doesn't work
With that formatting in place we generate a query that includes
both timestamps are in the same timezone but only one of them gets timezone fangled by HogQL
event.timestampis 22:31:01 but gets shifted 8 hours back from thereso they end up unmatched and the query fails even though it is absolutely 100 percent there
no bueno...
let's just make the date range wide enough that timezones can't cause a problem