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

Defining default decimal point for non JSON table LINST instruments #9277

Merged

Conversation

regisoc
Copy link
Contributor

@regisoc regisoc commented May 23, 2024

Brief summary of changes

This PR forces the decimal(14,4) (10 digits on the integer part, and 4 digits on the floating point part), instead of decimal(10,0) which is the default for numeric and was truncating floating point digits.
It only applies this change for instruments that require their own table (non JSON) and have numeric attribute.

Testing instructions (if applicable)

  1. use tools/generate_tables_sql.php and tools/generate_tables_sql_and_testNames.php with a non JSON LINST instrument with at least a numeric type line.
  2. the resulting SQL should be a valid SQL code that add a decimal(14,4) type for the numeric attribute, instead of a numeric attribute.

Link(s) to related issue(s)

Resolves #9237

@regisoc regisoc requested a review from driusan May 23, 2024 14:55
@regisoc regisoc marked this pull request as ready for review May 23, 2024 15:00
@ridz1208
Copy link
Collaborator

@driusan I was thinking we could have a (14,4) default which would be backwards compatible with the old (10,0) default but also allow the LINST standard to define the number of digits and number of decimals in the front end? just likesome fields have min and max?

@regisoc
Copy link
Contributor Author

regisoc commented May 24, 2024

@driusan @ridz1208 I like that idea. I can add that as a new configuration if that is ok for both of you.

@driusan
Copy link
Collaborator

driusan commented Jun 3, 2024

@regisoc I'm not sure how the option would work but it sounds good to me. (I think the first part of the (x, y) can be derived from the min/max that's already there with something like log10(max(abs(min), abs(max)))) + y.

Should I merge this and you'll do it in another PR or do you want to do it in this one?

@regisoc
Copy link
Contributor Author

regisoc commented Jun 4, 2024

@driusan @ridz1208 I think it can be merged as is.
The option can be added later as it will only impact both generate_table... scripts.

@driusan driusan merged commit 7b53bff into aces:main Jun 6, 2024
28 checks passed
@ridz1208 ridz1208 added this to the 26.0.0 milestone Jun 6, 2024
maximemulder pushed a commit to maximemulder/Loris that referenced this pull request Sep 25, 2024
…#9277)

This forces the decimal(14,4) (10 digits on the integer part, and 4 digits on the floating point part), instead of decimal(10,0) which is the default for numeric and was truncating floating point digits.
It only applies this change for instruments that require their own table (non JSON) and have numeric attribute.

Resolves aces#9237
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.

Numeric LINST types without json data enabled can not save decimals
3 participants