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.

Sign up for GitHub

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

Jump to bottom

Add checkExternalDecimal API #18272

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Add checkExternalDecimal API #18272

merged 1 commit into from
Apr 9, 2024

Conversation

VermaSh
Copy link
Contributor

@VermaSh VermaSh commented Oct 12, 2023

No description provided.

@VermaSh VermaSh force-pushed the daa_check_zonedDecimals branch from a07234f to 1097dd7 Compare October 12, 2023 13:22
@VermaSh
Copy link
Contributor Author

VermaSh commented Oct 12, 2023

Marking this as WIP as I need to update getExternalByteCounts call sites to use the new ExternalDecimalSignLocation enum.
cc: @joransiu @r30shah

@VermaSh VermaSh force-pushed the daa_check_zonedDecimals branch 2 times, most recently from 7a0dfe2 to 53c606a Compare October 23, 2023 18:46
@VermaSh VermaSh force-pushed the daa_check_zonedDecimals branch from 6a555f1 to 411fbe3 Compare December 15, 2023 20:52
@VermaSh VermaSh force-pushed the daa_check_zonedDecimals branch 7 times, most recently from 7ba8ffd to e3625f7 Compare January 10, 2024 02:21
@VermaSh VermaSh mentioned this pull request Jan 10, 2024
Merged
@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 10, 2024 •
edited
Loading

launched a personal build to test my changes

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 17, 2024

The test build passed without any failures. adoptium/openj9-systemtest#152 (comment). @r30shah this is ready for review.

@VermaSh VermaSh changed the title WIP: Add checkExternalDecimal API Add checkExternalDecimal API Jan 17, 2024
r30shah
r30shah requested changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

Can you add more information about this change in the Commit? Basically I would more information about checkExternalDecimal API and potential use cases.

jcl/src/openj9.dataaccess/share/classes/com/ibm/dataaccess/ExternalDecimal.java Outdated
* @param decimalType
* constant indicating the type of the decimal
*
* @return the condition code: 0 if all digit codes and the sign valid, 1 if the sign is invalid,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you write the condition codes in separate line so it is easier to read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated it in my latest commit.

jcl/src/openj9.dataaccess/share/classes/com/ibm/dataaccess/ExternalDecimal.java Outdated
if (precision < 1)
throw new IllegalArgumentException("Illegal Precision.");

if ((offset + (CommonData.getExternalByteCounts(precision, decimalType)) > byteArray.length) || (offset < 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

In which case we can have offset less than zero ? Asking because, if caller can set the offset to negative and first check passes, we would not do second check. Just a question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, that should really be 2 separate checks. I have updated it in my latest commit.

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 25, 2024

@r30shah this is ready for an other review.

@VermaSh VermaSh force-pushed the daa_check_zonedDecimals branch 2 times, most recently from 21a91ef to e96fbe5 Compare February 6, 2024 20:59
@VermaSh
Copy link
Contributor Author

VermaSh commented Feb 6, 2024 •
edited
Loading

Launched personal build to verify changes. I'll remove WIP tag once I have verified the changes.

@VermaSh VermaSh changed the title Add checkExternalDecimal API WIP: Add checkExternalDecimal API Feb 6, 2024
r30shah
r30shah requested changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

Minor cosmetic changes, Overall changes looks OK to me

jcl/src/openj9.dataaccess/share/classes/com/ibm/dataaccess/ExternalDecimal.java Outdated
* constant indicating the type of the decimal
*
* @return the condition code:
* 0 if all digit codes and the sign valid,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* 0 if all digit codes and the sign valid,
* 0 if all digit codes and the sign are valid,

jcl/src/openj9.dataaccess/share/classes/com/ibm/dataaccess/ExternalDecimal.java Outdated
* @return the condition code:
* 0 if all digit codes and the sign valid,
* 1 if the sign is invalid,
* 2 if at least one digit code is invalid, and
Copy link
Contributor

Choose a reason for hiding this comment

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

I think , and is not needed

jcl/src/openj9.dataaccess/share/classes/com/ibm/dataaccess/ExternalDecimal.java Outdated
* constant indicating the type of the decimal
*
* @return the condition code:
* 0 if all digit codes and the sign valid,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we take advantage of final int like we are doing in Decimal data to make this more readable ?

@VermaSh VermaSh force-pushed the daa_check_zonedDecimals branch 2 times, most recently from 1776ceb to 81228e2 Compare February 21, 2024 21:41
@VermaSh VermaSh changed the title WIP: Add checkExternalDecimal API Add checkExternalDecimal API Feb 22, 2024
@r30shah r30shah self-assigned this Mar 28, 2024
r30shah
r30shah requested changes Mar 28, 2024
View reviewed changes
jcl/src/openj9.dataaccess/share/classes/com/ibm/dataaccess/ExternalDecimal.java Outdated
* @param precision
* number of digits to be verified.
* @param decimalType
* constant indicating the type of the decimal. Supporte values are
Copy link
Contributor

Choose a reason for hiding this comment

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

typo here. Also as we are mentioning the constant from DecimalData, may be we should add that.

@r30shah
Copy link
Contributor

r30shah commented Mar 28, 2024

@VermaSh What is the order for the changes to go? I would assume this API need to go first and than adoptium/openj9-systemtest#152 right ?

@VermaSh VermaSh force-pushed the daa_check_zonedDecimals branch from 81228e2 to 738d50f Compare April 3, 2024 14:48
@VermaSh
Copy link
Contributor Author

VermaSh commented Apr 3, 2024

Yup, this needs to go ahead of adoptium/openj9-systemtest#152. Also, I have updated the PR with your suggestions.

@VermaSh VermaSh force-pushed the daa_check_zonedDecimals branch from 738d50f to f3da656 Compare April 3, 2024 18:23
r30shah
r30shah approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

LGTM

@r30shah
Copy link
Contributor

r30shah commented Apr 3, 2024

jenkins test sanity all jdk8,jdk11,jdk17

@r30shah
Copy link
Contributor

r30shah commented Apr 5, 2024

@VermaSh Can you verify the failures seen on x86 and aarch64?

@VermaSh
Copy link
Contributor Author

VermaSh commented Apr 8, 2024

@r30shah x86 failures were criu related and arm failure was due to a time out.

@r30shah
Copy link
Contributor

r30shah commented Apr 8, 2024

jenkins test sanity xlinux jdk11,jdk17

@VermaSh
Copy link
Contributor Author

VermaSh commented Apr 9, 2024

aarch64 cmdLineTester_getPid_0_FAILED test failure doesn't seem to be related. Here's the issue investigating same failure on other platforms.

@r30shah
Copy link
Contributor

r30shah commented Apr 9, 2024

jenkins test sanity alinux64 jdk17

@r30shah
Copy link
Contributor

r30shah commented Apr 9, 2024

Thanks @VermaSh for triaging these failures. I have launched last one that failed. Will merge the change once the build finishes.

@r30shah
Copy link
Contributor

r30shah commented Apr 9, 2024

All test passes. Merging the changes for DAA API

@r30shah r30shah merged commit b9b8221 into eclipse-openj9:master Apr 9, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r30shah r30shah r30shah approved these changes

Assignees

@r30shah r30shah

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@VermaSh @r30shah