Skip to content
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

[PART 2] pgx v5 support (#1823) #1874

Merged
merged 5 commits into from
Nov 17, 2022
Merged

Conversation

mcdoker18
Copy link
Contributor

Added support for v5 of the pgx Go drives

  • 'pgx/v5' option requires '--experimental' cli flag.
  • array, ranges, multiranges uses generic version for 'pgx/v5'. It supports multidemensional arrays.
  • 'pgx/v5' implementation prefers use pgtype types. For example, pgtype.Bool instead of sql.NullBool.
  • added support for new pg types:
      * datemultirange
      * tsmultirange
      * tstzmultirange
      * nummultirange
      * int4multirange
      * int8multirange
      * bit
      * varbit
      * cid
      * oid
      * tid
      * circle
      * line
      * lseg
      * path
      * point
      * polygon

Copy link
Contributor

@JordanP JordanP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing unreasonable ! Good job :)

return "pgtype.Tsrange"
case SQLDriverPGXV5:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can confirm this is the right thing to do. pgtype.Range[pgtype.Timestamp] is the correct Go type.

@@ -122,6 +151,8 @@ func postgresType(req *plugin.CodeGenRequest, col *plugin.Column) string {

case "jsonb":
switch driver {
case SQLDriverPGXV5:
return "[]byte"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that []byte seems to be the right type here. Jackc said here that []byte and string can directly be used for json and jsonb. No need to a pgtype wrapper. but my experience has been that []byte better handles NULL values.

Copy link
Contributor Author

@mcdoker18 mcdoker18 Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, user can override default []byte type with some struct type.

For example:

type SomeDTOStruct struct {
	Value1 int.      `json:"value_1"`
	Value2 string `json:"value_2"`
}
      "overrides": [
        {
          "column": "some_table.jsonb_field",
          "go_type": "*example.com/db/dto.SomeDTOStruct"
        }
      ]

Then pgx v5 will marshals struct to JSON and vice versa automatically.

I can add this example to the docs, if it's not obvious behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like above mentioned syntax for absolute path ("go_type": "*example.com/db/dto.SomeDTOStruct") not working . Throwing error path not recognised in latest go version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@f1forhelp if you are sure that there is a bug in the sqlc you can create an issue with reproducible example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcdoker18 its working.

I used these json values.

      "overrides": [
        {
          "column": "events.event_price",
          "go_type": {
            "import":"test_proj/models/sqlc_json_model",
            "package": "sqlcjsonmodel",
            "type":"EventPrice"
          }
        }
      ],

Comment on lines 183 to 230
return "pgtype.CIDR"
case SQLDriverLibPQ:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this cases the build to break. Can we add it back in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@JordanP
Copy link
Contributor

JordanP commented Nov 8, 2022

I am interesting in this PR too. Anything I could do to help ?

* 'pgx/v5' option requires '--experimental' cli flag.
* array, ranges, multiranges uses generic version for 'pgx/v5'. It supports multidemensional arrays.
* 'pgx/v5' implementation prefers use pgtype types. For example, pgtype.Bool instead of sql.NullBool.
* added support for new pg types:
  * datemultirange
  * tsmultirange
  * tstzmultirange
  * nummultirange
  * int4multirange
  * int8multirange
  * bit
  * varbit
  * cid
  * oid
  * tid
  * circle
  * line
  * lseg
  * path
  * point
  * polygon
@mcdoker18
Copy link
Contributor Author

Hi @kyleconroy Can you please take a look again? I fixed all merge conflicts

@kyleconroy kyleconroy merged commit 3623ec8 into sqlc-dev:main Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants