Fixes #25713: ReportsExecution doesn't have timezone on all fields #5959
Conversation
| fr"table_name = " ++ Fragment.const("'" + testTable + "'"), | ||
| fr"column_name in (" ++ Fragment.const("'" + insertionDateColumn + "'") | ||
| ) ++ fr", " ++ Fragment.const("'" + complianceComputationDateColumn + "'") ++ fr")" |
There was a problem hiding this comment.
since it's a parameterized queries, it can just be interpolated within the fragment, no ?
fr"table_name = ${testTable}"There was a problem hiding this comment.
well, yes, but not in the ALTER TABLE part, so I wanted to make it very clear that we can't use the same things in both place by using the same construct with a different escaping rule.
| where table_name = """ ++ toQuotedFr(table) ++ fr" and column_name = " ++ toQuotedFr( | ||
| column | ||
| ) ++ fr""" and data_type = 'timestamp with time zone' |
There was a problem hiding this comment.
The table and column does not need to be constant fragment but can simply be interpolated, since they are standard parameterized arguments for the query, and not part of the SQL DML syntax,
for instance :
There was a problem hiding this comment.
Added and fixed the quoting convention - we always use ${}
|
|
||
| val alter: Fragment = fr"ALTER TABLE " ++ toFr(table) ++ fr" ALTER COLUMN " ++ toFr( | ||
| column | ||
| ) ++ fr" TYPE TIMESTAMP WITH TIME ZONE USING " ++ toFr(column) ++ fr" AT TIME ZONE 'UTC';" |
There was a problem hiding this comment.
I'm not sure for the 'UTC' timezone, in our Doobie Meta we write dates using no specific timezone and I thought the PG session current timezone is used (SHOW TIMEZONE;), so maybe 'UTC' is a special test value here ?
I could not test the behavior because I have no reports locally but I believe that if the timezone is something else than UTC we could end up assigning wrong values to the columns
There was a problem hiding this comment.
we don't any information about what was used to write these timezone, this is lost for ever. The server config can have changed. And since it's not really use, it doesn't not matter. UTC is a as correct value, as other and in case of problem it will be much more easy to guess what happened if we don't have to check for what was the timezone when the migration was done.
|
PR updated with a new commit |
|
Wow, yes I didn't merge the port in RudderConfig in the PR 😓 |
|
PR updated with a new commit |
fanf
force-pushed
the
bug_25713/reportsexecution_doesn_t_have_timezone_on_all_fields
branch
from
b0241b0
to
4da4bd3
Compare
https://issues.rudder.io/issues/25713