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

Support bytes #75

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support bytes #75

wants to merge 3 commits into from

Conversation

chein-huang
Copy link

translate bytes to 'VARBINARY' in trino so we can filter column which type is ‘VARBINARY’ by bytes

@cla-bot
Copy link

cla-bot bot commented Apr 12, 2023

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 cla@trino.io. 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

@@ -0,0 +1,18 @@
{
// Use IntelliSense to learn about possible attributes.
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this file

Copy link
Author

Choose a reason for hiding this comment

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

sry, this file removed now

case []byte:
return "", UnsupportedArgError{"[]byte"}
return "X'" + strings.ToUpper(hex.EncodeToString(x)) + "'", nil
Copy link
Member

Choose a reason for hiding this comment

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

This adds support for using []byte in query params, right? What about reading VARBINARY columns in query results?

Also, please update the README, there's a section there with a list of supported and not supported data types.

Copy link
Author

Choose a reason for hiding this comment

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

reading VARBINARY columns in query results we need to implement sql.Scanner interface.
like

type TrinoVARBINARY []byte

func (h *TrinoVARBINARY) Scan(src any) (err error) {
	switch s := src.(type) {
	case string:
		var bytes []byte
		bytes, err = base64.StdEncoding.DecodeString(s)
		if err == nil {
			*h = bytes
		}
	default:
		err = fmt.Errorf("unsupported type: %T", src)
	}
	return
}

Copy link
Member

Choose a reason for hiding this comment

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

Would to like to try in this PR? I know I'm asking to extend the scope of work, so it's also ok to do this in a follow up PR

Copy link
Author

Choose a reason for hiding this comment

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

I will do this in next PR. I hope to also support driver.Valuer interface

@cla-bot
Copy link

cla-bot bot commented Apr 13, 2023

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 cla@trino.io. 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

README.md Outdated
@@ -196,6 +196,7 @@ When passing arguments to queries, the driver supports the following Go data typ
* integers
* `bool`
* `string`
* `bytes`
Copy link
Member

Choose a reason for hiding this comment

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

Bytes or []byte?

Copy link
Author

Choose a reason for hiding this comment

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

changed to []byte now

README.md Outdated
@@ -255,7 +256,10 @@ will receive a `[]interface{}` slice, with values of the following types:
* `json.Number` for any numeric Trino types
* `[]interface{}` for Trino arrays
* `map[string]interface{}` for Trino maps
* `string` for other Trino types, as character, date, time, or timestamp
* `string` for other Trino types, as character, date, time, varbinary or timestamp
Copy link
Member

Choose a reason for hiding this comment

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

This section is about reading data from Trino. We discussed this in another comment that support for []byte will be added in a separate PR, so remove this for now

Copy link
Author

Choose a reason for hiding this comment

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

oh yes, it removed now

@cla-bot
Copy link

cla-bot bot commented Apr 13, 2023

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 cla@trino.io. 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

@nineinchnick
Copy link
Member

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Oct 8, 2023

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 cla@trino.io. 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
Copy link

cla-bot bot commented Oct 8, 2023

The cla-bot has been summoned, and re-checked this pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants