Skip to content

handle InvalidCodePointException and add a testcase#7734

Merged
martint merged 1 commit intotrinodb:masterfrom
JoJoBizarreAdventure:testChar
Oct 21, 2022
Merged

handle InvalidCodePointException and add a testcase#7734
martint merged 1 commit intotrinodb:masterfrom
JoJoBizarreAdventure:testChar

Conversation

@JoJoBizarreAdventure
Copy link
Copy Markdown
Contributor

@JoJoBizarreAdventure JoJoBizarreAdventure commented Apr 24, 2021

Inherit changes in https://github.com/trinodb/trino/pull/7731/commits
Merge all changes and testcase in one file
handling this issue #7597

fixes #7597

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Apr 24, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Apr 24, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

3 similar comments
@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Apr 24, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Apr 24, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Apr 24, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

Copy link
Copy Markdown
Member

@skrzypo987 skrzypo987 left a comment

Choose a reason for hiding this comment

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

Looks good.
Please rewrite commit message to something like "Handle InvalidCodePointException in CharacterStringCasts". No need to specify that tests are added.

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.

That comment seems unnecessary. The test is pretty straightforward

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.

Use Character.MIN_SURROGATE and Character.MAX_SURROGATE constants

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.

codepoint -> 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.

ok, I will fix it later

@sopel39 sopel39 requested a review from findepi April 26, 2021 09:18
@sopel39
Copy link
Copy Markdown
Member

sopel39 commented Apr 26, 2021

@JoJoBizarreAdventure please sign cla form

@findepi
Copy link
Copy Markdown
Member

findepi commented Apr 26, 2021

LGTM overall % @skrzypo987 's comments.

Would be good to update the commit message. Maybe

Fix saturated char/varchar cast for U&DFFF

@findepi findepi removed their request for review April 26, 2021 11:29
@JoJoBizarreAdventure
Copy link
Copy Markdown
Contributor Author

LGTM overall % @skrzypo987 's comments.

Would be good to update the commit message. Maybe

Fix saturated char/varchar cast for U&DFFF

Ok, I will squash the commit and update the commit message later

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Apr 27, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

1 similar comment
@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Apr 27, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@JoJoBizarreAdventure
Copy link
Copy Markdown
Contributor Author

I have updated the commit and changed the commit message.
Please check it.
And I wonder if I should change the hex number in testcase to Character.MIN_SURROGATE too.

@skrzypo987
Copy link
Copy Markdown
Member

And I wonder if I should change the hex number in testcase to Character.MIN_SURROGATE too.

I'd leave the test case as it is.

Copy link
Copy Markdown
Member

@skrzypo987 skrzypo987 left a comment

Choose a reason for hiding this comment

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

Please fill out the CLA, and send a photo/scan to cla@trino.io. Without that we are unable to merge the PR.

@JoJoBizarreAdventure
Copy link
Copy Markdown
Contributor Author

Please fill out the CLA, and send a photo/scan to cla@trino.io. Without that we are unable to merge the PR.

I have sent one these days and waiting for reply now.

@JoJoBizarreAdventure
Copy link
Copy Markdown
Contributor Author

I have sent a CLA request but the recent commit doesn't add me to the contributors. Shall I send it again? In usual case how long will the response take?

@sopel39
Copy link
Copy Markdown
Member

sopel39 commented May 4, 2021

@cla-bot check

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented May 4, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented May 4, 2021

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

@martint
Copy link
Copy Markdown
Member

martint commented May 5, 2021

@JoJoBizarreAdventure, can you try re-sending your CLA? We never received it.

@JoJoBizarreAdventure
Copy link
Copy Markdown
Contributor Author

@JoJoBizarreAdventure, can you try re-sending your CLA? We never received it.

Ok,I will re-sending one with gmail, maybe changing the email will work.

@JoJoBizarreAdventure
Copy link
Copy Markdown
Contributor Author

@JoJoBizarreAdventure, can you try re-sending your CLA? We never received it.

I have re-sent one, please check.
image

@sopel39
Copy link
Copy Markdown
Member

sopel39 commented May 6, 2021

@cla-bot check

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented May 6, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented May 6, 2021

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

@JoJoBizarreAdventure
Copy link
Copy Markdown
Contributor Author

@cla-bot check

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented May 7, 2021

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

@cla-bot cla-bot bot added the cla-signed label May 7, 2021
@JoJoBizarreAdventure
Copy link
Copy Markdown
Contributor Author

I have cla-signed now, please check.

@JoJoBizarreAdventure
Copy link
Copy Markdown
Contributor Author

All checks pass now, it seems my work is done.

@martint martint merged commit 325ddf2 into trinodb:master Oct 21, 2022
@github-actions github-actions bot added this to the 401 milestone Oct 21, 2022
@colebow
Copy link
Copy Markdown
Member

colebow commented Oct 25, 2022

@martint - do we want a release note for this?

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.

Saturated floor cast from varchar to char may contain illegal characters

6 participants