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

sql: use ScalarType rather than mz_pgrepr::Type as param type #10619

Merged
merged 2 commits into from
Feb 13, 2022

Conversation

benesch
Copy link
Member

@benesch benesch commented Feb 13, 2022

The mz_pgrepr::Type is a PostgreSQL implementation detail but had
leaked into the Materialize-specific sql and coord crates. Correct
that boundary violation. The code to take a mz_pgrepr::Value into a
Datum gets much simpler in the process; the pgrepr::Type type is now
directly convertible to a ScalarType without looking at the datums
involved, so we can massively simplify the signature of
Value::into_datum and entirely remove pgrepr::null_datum.

Motivation

  • This PR refactors existing code.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR adds a release note for any user-facing behavior changes.

@benesch benesch force-pushed the pgrepr-type branch 3 times, most recently from 11832dc to 813c2b8 Compare February 13, 2022 02:05
The `mz_pgrepr::Type` is a PostgreSQL implementation detail but had
leaked into the Materialize-specific `sql` and `coord` crates. Correct
that boundary violation. The code to take a `mz_pgrepr::Value` into a
`Datum` gets much simpler in the process; the `pgrepr::Type` type is now
directly convertible to a `ScalarType` without looking at the datums
involved, so we can massively simplify the signature of
`Value::into_datum` and entirely remove `pgrepr::null_datum`.
The length of a character value is a property of the type, not the
value.
@benesch benesch merged commit 0726337 into MaterializeInc:main Feb 13, 2022
@benesch benesch deleted the pgrepr-type branch February 13, 2022 03:15
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

1 participant