Skip to content

Fix slice serialization#14320

Merged
arhimondr merged 2 commits intotrinodb:masterfrom
arhimondr:fix-slice-serialization
Sep 28, 2022
Merged

Fix slice serialization#14320
arhimondr merged 2 commits intotrinodb:masterfrom
arhimondr:fix-slice-serialization

Conversation

@arhimondr
Copy link
Copy Markdown
Contributor

Description

Fix Slice to Json serialization for non UTF-8 values

Non-technical explanation

N/A

Release notes

(X) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Sep 27, 2022
@arhimondr
Copy link
Copy Markdown
Contributor Author

I tried to comment out slice serialization in TrinoMainModule and ran a couple of integration tests. It doesn't seem to break anything thus it looks like it was being unused. However it breaks statistics delivery introduced in #14072

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this commit also remove the SliceSerialization.* classes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The SliceSerialization is needed by the #14072 that was introduced recently.

It doesn't seem to break anything thus it looks like it was being unused.

By that I meant that it looks like it was not used before #14072

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Serializing to a UTF string doesn't work for all possible values

just "a String"

"values" -> "codepoints" ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically it is still serialized into a String (that contains only base64 characters). By values I meant all possible values that a Slice can contain

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know what "UTF string" is.

let's change commit message to sth like

Interpreting as UTF-8 works only for UTF-8 encoded text (and even then not necessarily for all the codepoints), 
but a Slice can carry arbitrary bytes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, let me change the commit message

Interpreting as UTF-8 works only for UTF-8 encoded text
(and even then not necessarily for all the codepoints),
but a Slice can carry arbitrary bytes.
@arhimondr arhimondr force-pushed the fix-slice-serialization branch from fe24158 to efd921f Compare September 28, 2022 15:04
@arhimondr arhimondr merged commit 6b7cf93 into trinodb:master Sep 28, 2022
@arhimondr arhimondr deleted the fix-slice-serialization branch September 28, 2022 15:09
@github-actions github-actions bot added this to the 398 milestone Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants