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

feat(cube): support postgres cube #3188

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

jayy-lmao
Copy link
Contributor

I work on a sqlx codegen tool, and had an issue posted about the lack of support for the 'cube' extension type (https://www.postgresql.org/docs/current/cube.html).

jayy-lmao/sql-gen#9

I could not see any outstanding issues / PRs involving Cube, so can create an issue first if necessary for discussion.
Not particularly attached to how to structure the struct, serialisation, or deserialisation - ultimately just want to be able to support it for my sql-gen tool so that @yellowHatpro can carry on with using it for their project.

@jayy-lmao
Copy link
Contributor Author

jayy-lmao commented Apr 9, 2024

Sorry for the build jobs -- running locally the encoding seems to be fine.
Haven't been able to replicate locally with amd64 platform, alpine etc. Still figuring out why the encoding is different.

Update: have been able to reproduce by using

    let row = s.try_next().await?.unwrap();
    let rec = row.try_get::<PgCube, _>(0)?;

Locally to test. Will continue to investigate.

README.md Outdated Show resolved Hide resolved
@jayy-lmao
Copy link
Contributor Author

Have handled the text case, tests now passing ☺️

sqlx-postgres/src/type_checking.rs Outdated Show resolved Hide resolved
sqlx-postgres/src/types/cube.rs Outdated Show resolved Hide resolved
sqlx-postgres/src/types/cube.rs Outdated Show resolved Hide resolved
sqlx-postgres/src/types/cube.rs Outdated Show resolved Hide resolved
@jayy-lmao
Copy link
Contributor Author

Have made the changes to vector indexing :)

}
}

impl TryFrom<&str> for PgCube {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a FromStr impl.

It's fine to have both, but it's surprising to have a TryFrom<&str> impl and not a FromStr impl.

The only reason to do that is if Self wants to borrow from the string (think zero-copy parsing) which the FromStr trait doesn't support due to its lack of a lifetime bound.

@jayy-lmao
Copy link
Contributor Author

jayy-lmao commented Apr 24, 2024

Have made changes

  • trimming the input string to make it more flexible for the spec & for user input
  • allow for _cube array type (And included a test for it)
  • remove TryFrom behaviour for &[u8], using direct function instead
  • avoid double creating a buffer, but just writing directly to the PgArgumentBuffer

@jayy-lmao
Copy link
Contributor Author

Apologies, I've been away for a little - I've addressed all comments but if there's any other changes that are wanted let me know

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.

None yet

2 participants