-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[CALCITE-7398] Incorrect int cast in VariantNonNull#cast for BIGINT #4771
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
Conversation
| "INTEGER ARRAY"); | ||
| f.checkScalar("cast(cast('abc' as VARIANT) AS VARCHAR)", "abc", "VARCHAR"); | ||
| f.checkScalar("cast(cast('abc' as VARIANT) AS CHAR(3))", "abc", "CHAR(3)"); | ||
| f.checkScalar("cast(cast(2147483648 as VARIANT) as DECIMAL)", "2147483648", "DECIMAL(19, 0)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the bug fix way is OK,but please note:
- Your commit message should not contain words like "Fix," and you need to modify the Jira link content to match the PR.
- It is recommended to add the corresponding JIRA info to the
testCastToBooleanmethod, or you can add the corresponding comment here.
|
@qianheng-aws thanks for the patch. Per our contributing guidelines, when fixing an issue, the Jira/PR title (and the commit message) should describe the problem (and not begin with "Fix ..."), e.g. |
Signed-off-by: Heng Qian <qianheng@amazon.com>
xuzifu666
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the fix is acceptable.
I've encountered this CI error a few times before; it's sporadic and keeps recurring even after multiple reruns. I've documented it on a JIRA: https://issues.apache.org/jira/browse/CALCITE-7399.
This PR has minor changes; if you don't mind, you could resubmit a new pr to make CI happy.
xiedeyantu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
I don't think we need to submit a new PR. This CI error doesn't seem to be related to the PR, so we can merge it in without worry. |
The CI error is indeed unrelated to changes. If there are no other comments in 24 hours, it will be merged. |
CALCITE-7398