fix(#144): Handle null date in telemetry doc #145
Conversation
| '1' | ||
| END | ||
| )::date AS period_start, | ||
| (CASE |
There was a problem hiding this comment.
I don't think migrations run twice, so you'll have to create new which REPLACE this view instead of editing this definition. Correct me if I'm wrong.
You can test by:
- Deploying production couch2pg. Seeing this bug
- Upgrading to this version.
- Is it fixed?
I believe the migraiton won't re-run on upgrade...
Hopefully we can replace the definition and not drop... because that causes a cascade drop :(
There was a problem hiding this comment.
Thanks, I've made the changes.
I had to use drop cascade since materialized views can not be replaced like view CREATE OR REPLACE.
There was a problem hiding this comment.
that's pretty scary then... who knows what we are going to cascade delete...
i'm not sure how we message this - but i think we need to be really clear about this somehow. can you ask on #development or forum or something?
| ELSE | ||
| (doc#>>'{metadata,month}')::int --> Month is between 1 - 12 | ||
| END, | ||
| CASE --> day of the month, else 1 |
There was a problem hiding this comment.
i know this isn't your code, but cleaner to use COALESCE here instead of CASE
There was a problem hiding this comment.
I moved this code to a function so that it's more readable and single place to fix if any issue.
| END | ||
| )::date AS period_start, | ||
| (CASE | ||
| WHEN doc#>>'{metadata,year}' IS NULL THEN '1970-1-1' |
There was a problem hiding this comment.
can this be null instead of random date?
There was a problem hiding this comment.
Hi @kennsippell , There'll be difference in results when using NULL vs 1970-1-1. Since we have done DISTINCT ON (doc #>> '{metadata,deviceId}', doc #>> '{metadata,user}'), If we put NULL on our field, the NON-NULL value will be selected by default which in our case is first record and old record. If we put 1970-1-1, it will consider the latest data aailable for those user and device. Since in our case, most of the records with null are recent, we will not be creating metadata for recent numbers.
With this, there'll be two cases.
- Keep NULL, and get older records, most likely prior to 4.5.0 upgrades, period_start will have good older date and old data will be available.
- Keep 1970-1-1 and get recent record for that user and device.
period_startwill be 1970-1-1 in this case and latest data available.
Let me know what'd you prefer here.
There was a problem hiding this comment.
IC. The distinct on should pick the first value ordered by device_id, user_name, period_start. NULL is always sorted last by postgres. 1970 is likely also always going to be last, so they seem pretty close in functionality but not identical (did i miss something?)
A third option is that we could use the 4.5.0 release date, since we know the telemetry doc is "at least that old" ? Nov-20-2023. Maybe that is getting too weird.
I think all three are decent options. You probably know best since you're deeper in the data atm. I'd likely go with NULL just to avoid "magic values" - but have no strong opinion and would respect either choice.
There was a problem hiding this comment.
Since we already have issue on cht-core to fix this problem of null telemetry dates, it doesn't really matter once that's fixed on what we've put here for useview_telemetry_devices. For other views, I opted to go with NULL.
|
Closing this Pull request without merging since the issue was with Nepali projects only and we wanted to avoid drop cascade. Any projects having this issue can apply changes on this PR. |
Fix #144 by assigning default date of
1970-1-1where dates are not available.