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.

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

[UIST] Field Sort Priority adjustment to larger value #641

Merged
merged 12 commits into from
Aug 5, 2024

Conversation

ViktoriaFreidel
Copy link
Contributor

No description provided.

@Markus1812
Copy link
Member

Hi Viktoria, it seems like you reverted your commits. The diff between your fork and this repository is empty, as you can see in the "Files changed" tab in this PR. Can you please try to commit your changes again?

@ViktoriaFreidel
Copy link
Contributor Author

Hi Viktoria, it seems like you reverted your commits. The diff between your fork and this repository is empty, as you can see in the "Files changed" tab in this PR. Can you please try to commit your changes again?

HI, I commited changes again, but check failed. Can I fix they somehow?

@Markus1812
Copy link
Member

Markus1812 commented Jul 24, 2024

Hi Viktoria, it seems like you reverted your commits. The diff between your fork and this repository is empty, as you can see in the "Files changed" tab in this PR. Can you please try to commit your changes again?

HI, I commited changes again, but check failed. Can I fix they somehow?

You can ignore these two check failures for now, they are related to checking if the change is compatible. I am not sure whether your change is compatible (see our wiki) but I will clarify this with my colleagues. I will come back to you soon.

@Markus1812
Copy link
Member

Hi, is your object type already used externally (except of ADT)?

Our main concern is when importing an AFF object with 5 decimals into a system which is still on the old change (with 3 decimals) our transformation will break, as it cannot write a number with 5 decimals into a field with only 3 decimals. But if it is only used in ADT, it should be fine.

@ViktoriaFreidel
Copy link
Contributor Author

ViktoriaFreidel commented Jul 29, 2024

Hi, is your object type already used externally (except of ADT)?

Our main concern is when importing an AFF object with 5 decimals into a system which is still on the old change (with 3 decimals) our transformation will break, as it cannot write a number with 5 decimals into a field with only 3 decimals. But if it is only used in ADT, it should be fine.

Hi, UIST Object type is used for import/export of objects in abapGit. When is trying to push Object with 5 decimals into 3 decimals (defined in older system), the error log is display (e.g. The number 9999.99999 of field "sortPriority" has too many decimals. 3 decimals allowed) and UIST Object not imported into system. Content can be adapted on Git repository and import via abapGit restarted.

@Markus1812
Copy link
Member

Hi, thank you for answering all my questions. I have consulted my colleagues, and we came to the following conclusion:

In general, such a change would not be compatible, as you have already described:

When is trying to push Object with 5 decimals into 3 decimals (defined in older system), the error log is display (e.g. The number 9999.99999 of field "sortPriority" has too many decimals. 3 decimals allowed) and UIST Object not imported into system.

In this case, our preferred way would be to increase the format version, so the importing system (old system without the change) would recognize that the object has a newer version than it currently understands.

As your specific object is currently only used in the cloud (as UIST has no on-prem release yet), we can accept the change without increasing the format version.

@larshp
Copy link
Collaborator

larshp commented Aug 1, 2024

schema generation now running, thanks to open-abap/open-abap-core#893

@Markus1812
Copy link
Member

Okay, some updates regarding the failing Github Action Generate JSON Schema:

The assertion mentioned in the error log occurs because of some implementation in open-abap-core calculating the max length of a packed number. This error was fixed with open-abap/open-abap-core#893.

/home/runner/work/abap-file-formats/abap-file-formats/generate/node_modules/@abaplint/runtime/build/src/statements/assert.js:6
        throw new Error("ASSERTION_FAILED");
              ^

Error: ASSERTION_FAILED
    at Statements.assert (/home/runner/work/abap-file-formats/abap-file-formats/generate/node_modules/@abaplint/runtime/build/src/statements/assert.js:6:15)
    at cl_abap_exceptional_values.get_max_value (file:///home/runner/work/abap-file-formats/abap-file-formats/generate/output/cl_abap_exceptional_values.clas.mjs:43:25)

During this investigation we stumbled across the max value definition for packed numbers:
A packed number with length 5 decimals 3 consists of 5 * 2 - 1 = 9 digits in total from which 3 are reserved for decimal digits. The rest 9 - 3 = 6 digits can be used before the decimal point resulting in the max value of 999,999.999.

If you now want to increase the decimal part of the number to 5 decimals, it would also be possible to set the packed number to length 5 decimals 5 (which looks odd) but allows for the max value of 9,999.99999. This is completely fine for your defined maximum of $maximum: 9999.99999 and would save some space.

In contrast, length 9 decimals 5 allows the maximum value of 999,999,999,999.99999 which is a lot.

So this is now up to you to decide @ViktoriaFreidel: We would recommend to reduce the length of sort_priority to length 5 decimals 5 but this is up to you.

@ViktoriaFreidel
Copy link
Contributor Author

Okay, some updates regarding the failing Github Action Generate JSON Schema:

The assertion mentioned in the error log occurs because of some implementation in open-abap-core calculating the max length of a packed number. This error was fixed with open-abap/open-abap-core#893.

/home/runner/work/abap-file-formats/abap-file-formats/generate/node_modules/@abaplint/runtime/build/src/statements/assert.js:6
        throw new Error("ASSERTION_FAILED");
              ^

Error: ASSERTION_FAILED
    at Statements.assert (/home/runner/work/abap-file-formats/abap-file-formats/generate/node_modules/@abaplint/runtime/build/src/statements/assert.js:6:15)
    at cl_abap_exceptional_values.get_max_value (file:///home/runner/work/abap-file-formats/abap-file-formats/generate/output/cl_abap_exceptional_values.clas.mjs:43:25)

During this investigation we stumbled across the max value definition for packed numbers: A packed number with length 5 decimals 3 consists of 5 * 2 - 1 = 9 digits in total from which 3 are reserved for decimal digits. The rest 9 - 3 = 6 digits can be used before the decimal point resulting in the max value of 999,999.999.

If you now want to increase the decimal part of the number to 5 decimals, it would also be possible to set the packed number to length 5 decimals 5 (which looks odd) but allows for the max value of 9,999.99999. This is completely fine for your defined maximum of $maximum: 9999.99999 and would save some space.

In contrast, length 9 decimals 5 allows the maximum value of 999,999,999,999.99999 which is a lot.

So this is now up to you to decide @ViktoriaFreidel: We would recommend to reduce the length of sort_priority to length 5 decimals 5 but this is up to you.

Hi, we will change sort_priority to `length 5 decimals 5 as suggested. Thank you for clarifying.

Copy link
Member

@Markus1812 Markus1812 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 to me, thank you.

@Markus1812 Markus1812 merged commit 486b146 into SAP:main Aug 5, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants