Support VARBINARY
#135
Support VARBINARY
#135
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
| }, func(hc *docker.HostConfig) { | ||
| hc.Ulimits = []docker.ULimit{ | ||
| { | ||
| Name: "nofile", | ||
| Hard: 4096, | ||
| Soft: 4096, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Trino requires a minimum of 4096 descriptors in order to startup. On my linux machine, I found docker defaults to a limit of 1024.
I've updated the test configuration such that we can be sure the container is configured with reasonable ulimits.
nineinchnick
left a comment
nineinchnick
left a comment
There was a problem hiding this comment.
Looks great! Once the CLA is cleared, I can merge it. Please remove prefixes from commit messages.
hudclark
force-pushed
the
hudclark/support-varbinary
branch
from
321f328 to
d67f2d8
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Appreciate the quick review @nineinchnick! I've removed the commit message prefixes and updated the readme to resolve that strange formatting. I also want to call out that this might be considered a breaking change for clients who have previously been reading in |
nineinchnick
added
breaking-change
|
I will investigate and fix the failing test shortly |
hudclark
force-pushed
the
hudclark/support-varbinary
branch
from
d67f2d8 to
cefa075
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
@cla-bot check |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Hey @nineinchnick, looks like I've been added now trinodb/cla@4053dbd. Mind trying again? Thanks! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
2 participants
Overview
This PR adds support for encoding/decoding the
[]bytego type asVARBINARYdata.Additional Information
To keep the scope of this PR small, I did not add support for reading
VARBINARYvalues within complex types. This is not a regression, but a gap in the serialization/deserialization code. Timestamps, Numbers, etc also have the same issue - they are read into go types right off of the decoded JSON. This means that aROW(X'68656C6C6F')is deserialized as a[]interface{}{"aGVsbG8="}. I did update the README to document this behavior.To showcase this behavior, I've added a (skipped) failing test case. Given the behavior is documented, changing it would be a breaking change. Perhaps an option could be added in the future to recursively parse complex types such that this test case passes?
I'm happy to drop the commit with the skipped test case if we feel it adds noise or does not represent the desired state.
Resolves:
Note
I have signed and emailed an individual CLA.