-
Notifications
You must be signed in to change notification settings - Fork 802
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
pg-gen: make sqlc-pg-gen the complete source of truth for pg_catalog.go #1809
Conversation
@kyleconroy I think given the fact that you have been pretty diligent about adding endtoend tests whenever you change something, I think as long as this passes the test, it should be safe. But if you can remember any other manual edits to pg_catalog as well, we'll definitely need to fix them - this could possibly be a breaking change if we don't catch one of those edits. |
This is needed for the end-to-end tests, at least until resolving a function call is smart enough to match functions parameter types
@@ -79,7 +79,7 @@ ORDER BY bucket DESC | |||
type ListMetricsRow struct { | |||
Bucket interface{} | |||
CityName sql.NullString | |||
Avg string | |||
Avg float64 |
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.
The query is:
SELECT time_bucket('15 days', time) AS bucket, city_name, AVG(temp_c)
FROM weather_metrics
WHERE DATE_SUB(NOW(), INTERVAL 6 MONTH)
GROUP BY bucket, city_name
ORDER BY bucket DESC;
So I think the float64 is actually correct here - the string was incorrect before. They are also "both wrong" since the order of the function definitions in pg_catalog.go
is actually what is determining the types here.
@@ -104,7 +104,7 @@ func Pgcrypto() *catalog.Schema { | |||
Name: "digest", | |||
Args: []*catalog.Argument{ | |||
{ | |||
Type: &ast.TypeName{Name: "text"}, | |||
Type: &ast.TypeName{Name: "bytea"}, |
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.
Changed the order of these, as they affected the end-to-end tests, and just wanted to preserve legacy behavior.
}, | ||
}, | ||
ReturnType: &ast.TypeName{Name: "text"}, | ||
}, |
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.
This concat is not in the pg_catalog on the main branch, so I removed it here too.
}, | ||
}, | ||
ReturnType: &ast.TypeName{Name: "numeric"}, | ||
}, |
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.
Simply changed the order of the functions here to preserve the behavior of the e2e tests.
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.
Wow, amazing work. I'm kicking myself for editing this file manually over the last year. Especially appreciate the work at preserving test compatibility.
This is sort of a yak-shaving task for #1748
Problem: there seem to have been some manual edits to pg_catalog.go over the course of time. This makes it hard (almost impossible actually) to do any change there with any confidence.
Solution: Do some detective work on the manual edits performed, and incorporate them into pg-gen. Once that is done the work on #1748 will be a lot easier.
Here are the 2 manual edits as far as I can tell:
9513155 (pg_catalog functions with variadic signatures - this is completely solved by this PR)
And
846c629 (manual edits to pg_catalog that I believe will be fixed "right way" once "direct args" vs "aggregated args" are separated in #1748 ).
Commits in order tell a story, probably best to review that way.