Skip to content

Test and fix cast from bigint to varchar#17152

Merged
highker merged 1 commit intoprestodb:masterfrom
v-jizhang:cast-to-varchar
Feb 7, 2022
Merged

Test and fix cast from bigint to varchar#17152
highker merged 1 commit intoprestodb:masterfrom
v-jizhang:cast-to-varchar

Conversation

@v-jizhang
Copy link
Copy Markdown
Contributor

@v-jizhang v-jizhang commented Jan 3, 2022

Cherry-pick of trinodb/trino#10090

Co-authored-by: kasiafi 30203062+kasiafi@users.noreply.github.com

Test plan - Added tests

== RELEASE NOTES ==

General Changes
* Test and fix cast from bigint to varchar

prithvip
prithvip approved these changes Jan 27, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@prithvip prithvip left a comment

Choose a reason for hiding this comment

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

Just some style nits

presto-main/src/test/java/com/facebook/presto/sql/TestExpressionInterpreter.java Outdated
Copy link
Copy Markdown
Contributor

@prithvip prithvip Jan 27, 2022

Choose a reason for hiding this comment

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

nit: Could this testing logic be put into a private static "assertThrows" (or something like that) helper method so as to match the style of the rest of this class? It could be helpful in the future too to test exceptions being thrown properly, and would be less awkward than catching an exception thrown by an assertion.

Copy link
Copy Markdown
Contributor Author

@v-jizhang v-jizhang Jan 31, 2022

Choose a reason for hiding this comment

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

I will refactor it in PR #17232

...in/src/test/java/com/facebook/presto/sql/planner/iterative/rule/TestSimplifyExpressions.java Outdated
Copy link
Copy Markdown
Contributor

@prithvip prithvip Jan 27, 2022

Choose a reason for hiding this comment

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

Same here as above

@prithvip prithvip requested a review from highker January 27, 2022 03:04
highker
highker approved these changes Jan 27, 2022
View reviewed changes
presto-main/src/test/java/com/facebook/presto/operator/scalar/AbstractTestFunctions.java Outdated
Copy link
Copy Markdown

@highker highker Jan 27, 2022

Choose a reason for hiding this comment

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

not used?

Copy link
Copy Markdown
Contributor Author

@v-jizhang v-jizhang Jan 31, 2022

Choose a reason for hiding this comment

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

It will be used in other similar PRs, like #17232

@highker highker self-assigned this Jan 27, 2022
@v-jizhang
Copy link
Copy Markdown
Contributor Author

v-jizhang commented Feb 2, 2022

I've run locally the failed tests and they pass.

@highker
Copy link
Copy Markdown

highker commented Feb 3, 2022

@v-jizhang can we rebase and force push?

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Feb 3, 2022

CLA Not Signed

@v-jizhang @kasiafi
Test and fix cast from bigint to varchar
d4ca4db 
Cherry-pick of trinodb/trino#10090

Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
@v-jizhang
Copy link
Copy Markdown
Contributor Author

v-jizhang commented Feb 3, 2022

@highker Done.

@highker highker merged commit 003ecf9 into prestodb:master Feb 7, 2022
This was referenced Feb 24, 2022
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@highker highker highker approved these changes

@prithvip prithvip prithvip approved these changes

Assignees

@highker highker

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@v-jizhang @highker @prithvip