-
Notifications
You must be signed in to change notification settings - Fork 100
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
Setup SQL server based on pgx frontend/backend protocol #4881
base: main
Are you sure you want to change the base?
Conversation
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.
Two overall comments:
- It's worth going over the PR and refactoring or clarifying use of the word "Postgres". There are many places where it's not clear that the code pertains to Postgres wire compatibility, as opposed to connecting to an external Postgres database. In some cases, a docstring would help, in other cases, it's worth considering using a term like
psql
orpgwire
instead ofpostgres
. - The code in
runtime/resolvers/postgres_sql.go
is pretty hard to navigate, despite the logic being fundamentally pretty simple. I suggest taking a fresh look at the code with a focus on simplifying and documenting the code. There's also a question (raised in one of the below comments) of whether this should even be a resolver, or if it's better implemented in a dedicated package for Postgres wire compatibility.
@@ -72,6 +74,7 @@ require ( | |||
github.com/rs/cors v1.9.0 | |||
github.com/santhosh-tekuri/jsonschema/v5 v5.2.0 | |||
github.com/sashabaranov/go-openai v1.19.3 | |||
github.com/shopspring/decimal v1.4.0 |
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.
Could we use the math/big
package directly and avoid this dependency?
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.
My understanding is that we need shopspring/decimal
to easily encode Numeric
: [https://github.com/jackc/pgx/issues/56
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.
Didn't go in depth here, but shopstring/decimal
is not a dependency of the pgx
project (based on its go.mod
file), so they must internally have found a different way around that?
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.
Sorry I posted the wrong link.pgx
recommends using shopspring/decimal
: https://github.com/jackc/pgx/wiki/Numeric-and-decimal-support
The default solution can only be used for float64
or string
.
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
closes #4817