Make queries valid postgresql #1260
Conversation
|
Preview this PR here: https://dev-docs.revenuecat.com/pr-1260/ |
There was a problem hiding this comment.
Pull request overview
This PR converts SQL queries to valid PostgreSQL syntax by replacing non-standard DATE_DIFF function calls with PostgreSQL-compatible alternatives. The changes ensure these queries, which were previously incompatible with PostgreSQL despite being labeled as such, can now execute correctly.
Key changes:
- Replaced
DATE_DIFFcalls withEXTRACT(EPOCH FROM ...)for timestamp calculations or simplified to direct timestamp comparisons where appropriate - Corrected column references from
pricetoprice_in_usdto match the actual schema - Replaced hardcoded dates with parameterized placeholders for better reusability
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scheduled-data-exports_1.pgsql | Removed redundant DATE_DIFF validation check |
| scheduled-data-exports_2.pgsql | Removed redundant DATE_DIFF validation check |
| scheduled-data-exports_4.pgsql | Removed redundant DATE_DIFF validation check |
| scheduled-data-exports_7.pgsql | Removed redundant DATE_DIFF validation check |
| scheduled-data-exports_9.pgsql | Removed redundant DATE_DIFF validation check |
| scheduled-data-exports_10.pgsql | Simplified DATE_DIFF check to direct timestamp comparison |
| scheduled-data-exports_11.pgsql | Converted DATE_DIFF to EXTRACT/EPOCH, fixed price column name, improved formatting |
| scheduled-data-exports_12.pgsql | Converted DATE_DIFF to EXTRACT/EPOCH, fixed price column name, parameterized hardcoded dates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@dpannasch can you double check some of the changes I made? I think I kept the logic the same, and tested them locally on real data, but just want another set of eyes on them :) |
| WHERE date(effective_end_time) > [targeted_date] | ||
| AND date(start_time) <= [targeted_date] | ||
| AND is_trial_period = 'false' | ||
| AND DATE_DIFF('s', start_time, end_time)::float > 0 |
There was a problem hiding this comment.
Generally looks good, but for the few queries where this filter is just removed, what's the line of reasoning?
There was a problem hiding this comment.
Yeah I was debating this, but I was thinking that if effective_end_time > date and start_time <= date, then it's start_time <= date < effective_end_time, so end_time - start_time is always > 0 and the condition is redundant. But I know that start_time > end_time does happen, if we want to keep this condition in symbolically, I'm totally ok with that
There was a problem hiding this comment.
Yeah the symbolic part I'm not concerned about at all, in fact this is one of those weird filters that I think is nonsensical to customers and is only in there because Charts does it that way. I'd maybe run a few test queries to verify that the same set of transactions that this filter excludes are also excluded when it's removed, but your logic for why it should makes sense to me.
Motivation / Description / Changes introduced
We technically say that these queries are Postgresql, but many queries weren't valid postgresql syntax so this fixes that plus fixes a couple typos and small logic simplifications.
Generally:
DATE_DIFFwas replaced byEXTRACT/EPOCHDATE_DIFF('s', start_time, end_time)::float > 0was replaced withend_time > start_timewhen possibleLinks to discussion, or Linear ticket (if applicable)
Additional comments