Add support for floating point operations #2742

smortex · 2019-05-21T21:48:46Z

This PR is about supporting math expression using floating-point values. The code is published for review for now, because some decisions might need to be discussed. I plan to amend the commits to address the issues and comments raised by the syslog-ng community, and remove the [Do not merge] tag when I consider the changes to be ready.

Fixes #2734

kira-syslogng · 2019-05-21T21:48:48Z

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

kira-syslogng · 2019-05-21T21:48:48Z

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

modules/basicfuncs/numeric-funcs.c

Kokan · 2019-05-21T21:52:07Z

@kira-syslogng ok to test

kira-syslogng · 2019-05-21T22:17:06Z

Build FAILURE

kira-syslogng · 2019-05-21T22:48:41Z

Build SUCCESS

kira-syslogng · 2019-05-21T23:43:04Z

Build SUCCESS

Kokan · 2019-05-23T05:46:39Z

@smortex I brough up this feature to the team, and we want to discuss it. It may take some time, but I hope not later than next Wednesday.

smortex · 2019-05-23T07:04:34Z

Sure, thanks you for taking care of it! I started "the easy way" and I guess a lot of topics will be raised, such as:

should sum, minimum, maximum & co be updated to support floats;
should the system be reworked to minimize the impact of roundings when the result of an operation is used in another operation;
etc.

Maintainers of the project can push commits to my branch, feel free to do so, but please send me a heads-up in this case so that I do not git push -f and loose your changes unexpectedly 😉

Kokan · 2019-05-29T07:30:41Z

I tried to summarise the result of that conversation:

A short summary of the discussion about this topic:

Re/defining the arithmetic operations:

The way how operation should be defined: like the one you proposed in your PR, the one in the issue( new operations for float).
The conclusion on that was that your proposal is the solution we want to have. (Mainly because of the next point, and because it is natural on some level.)

Impact on current configurations

With your version it is not an issue, as float was not supported before and operations with float resulted invalid/unwanted result (like NaN)

Storage method/1

There is a recurring idea that nv-pairs should have more sophisticated way to store different types (other then the string representation).
This discussion was halted as it is kinda complex, and that would explode the work needed to be done now (also could be splitted as well).

Storage method/2

Your proposal stores the float in a string representation. (this is somewhat connected to the next topic)
That introduces a precision syslog-ng could handle, which must be defined and compatible what float/double precision could do.

Operations error propagation

A brief discussion about the errors each operation could introduce, or just the converting into string.
The conclusion on this was that syslog-ng is not a scientific tool, and as such the error propagation is a best effort basis.
Meaning if some issue raises on the matter, we can open a discussion to solve or state that won't solve such an issue;
as likely such discussion will never come up.

What it means for this PR:

The way you defined the operation extension is OK
The precision/storage need some improvement

Kokan

Please in case of all [optional] comment either reply won't fix or do the change; I am fine with both.

modules/basicfuncs/numeric-funcs.c

modules/basicfuncs/tests/test_basicfuncs.c

modules/basicfuncs/numeric-funcs.c

kira-syslogng · 2019-05-31T22:09:55Z

Build SUCCESS

smortex · 2019-06-02T18:03:59Z

@Kokan, just to be sure, can you please confirm that your approval has value of "ok for squashing the commits and moving on to the next step"?

Kokan · 2019-06-02T18:11:52Z

@smortex yes for the squash; but if by next step you mean merging the PR, then no. as that requires at least two approver.

smortex · 2019-06-02T19:08:05Z

Sure! By "next step" I meant further work on the PR ;-)

Make it possible to use the mathematic operators with floating point values. Behave like the C programming language: - if both operands are integers, then compute and return an integer; - if any of the operands is a floating-point number, return a floating-point result. $(/ 3 2) #=> 1 $(/ 3.0 2) #=> 1.500000 $(/ 3 2.0) #=> 1.500000 $(/ 3.0 2.0) #=> 1.500000

With the ability to compute values with floating points, we need to be able to round results.

smortex · 2019-06-02T19:16:03Z

I reordered the commits, fixed and squashed the commits after rebasing on top of the master branch. I pushed the two modified commits one by one in order to trigger two builds. Let's see how is goes 😉

kira-syslogng · 2019-06-02T19:34:43Z

Build SUCCESS

smortex · 2019-06-03T13:13:53Z

Thanks! I added the optional precision argument to round, and reached a complete coverage of my use-case.

kira-syslogng · 2019-06-03T13:20:40Z

Build SUCCESS

kira-syslogng · 2019-06-03T18:10:56Z

Build SUCCESS

kira-syslogng · 2019-06-03T19:06:08Z

Build SUCCESS

modules/basicfuncs/numeric-funcs.c

modules/basicfuncs/tests/test_basicfuncs.c

modules/basicfuncs/numeric-funcs.c

smortex · 2019-06-04T07:35:48Z

I added a bunch of commit that address the problems you spotted. Thanks!

kira-syslogng · 2019-06-04T07:42:42Z

Build FAILURE

kira-syslogng · 2019-06-05T23:25:17Z

Build FAILURE

Kokan · 2019-06-06T12:48:49Z

@smortex please feel free to fixup the changes in the next step (if you agree with my comment on your commit), and from my side it is good to go.

Allow an optional second argument to the round template to set the precision of the rounding. The default is 0 (output a natural number) but values up to 20 are accepted. The limit of 20 was set in accordance with the precision used internally for computations.

kira-syslogng · 2019-06-06T20:25:50Z

Build SUCCESS

smortex · 2019-06-06T20:28:07Z

Thanks @Kokan I squashed the last commits into a single one. It looks like the "default" kira build fails, but I cannot access the system. Is there something I can fix myself?

Kokan · 2019-06-06T21:05:28Z

@smortex please ignore the default kira.

bazsi · 2019-06-07T04:05:52Z

That job is work in progress, so ignore it please. I have disabled it for now. Kókai Péter <notifications@github.com> ezt írta (időpont: 2019. jún. 6., Cs 23:05):

…

@smortex <https://github.com/smortex> please ignore the *default* kira. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2742>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFOK5VOZF5DRLKEHAASIP3PZF3ZTANCNFSM4HOO7YUQ> .

Kokan · 2019-06-07T06:07:54Z

(@smortex I'll do some stuff here to make default green, you still can ignore)

Kokan · 2019-06-07T06:08:41Z

@kira-syslogng retest this please

kira-syslogng · 2019-06-07T06:26:52Z

Build FAILURE

Kokan · 2019-06-12T05:32:34Z

@kira-syslogng retest this please

kira-syslogng · 2019-06-12T05:55:21Z

Build SUCCESS

bazsi

nice work!

smortex commented May 21, 2019

View reviewed changes

modules/basicfuncs/numeric-funcs.c Outdated Show resolved Hide resolved

Kokan added community-driven-merge labels May 21, 2019

smortex force-pushed the floating-point-operations branch from 6a6117c to 42e9ab9 Compare May 21, 2019 21:57

smortex force-pushed the floating-point-operations branch from 69f8e08 to c082655 Compare May 21, 2019 23:23

smortex mentioned this pull request May 22, 2019

Support for operation on floats #2734

Closed

Kokan added this to the syslog-ng-3.22 milestone May 29, 2019

Kokan suggested changes May 29, 2019

View reviewed changes

smortex force-pushed the floating-point-operations branch from 314e96e to 919909d Compare May 31, 2019 21:50

This comment has been minimized.

Sign in to view

Kokan previously approved these changes Jun 1, 2019

View reviewed changes

smortex added 2 commits June 2, 2019 09:12

Add round, ceil and floor numeric functions

61f42a5

With the ability to compute values with floating points, we need to be able to round results.

smortex dismissed Kokan’s stale review via 918c534 June 2, 2019 19:12

smortex force-pushed the floating-point-operations branch from 919909d to 918c534 Compare June 2, 2019 19:12

smortex force-pushed the floating-point-operations branch from 1b582ba to 4d6f1a0 Compare June 3, 2019 13:01

smortex changed the title ~~[Do not merge] Add support for floating point operations~~ Add support for floating point operations Jun 3, 2019

smortex force-pushed the floating-point-operations branch 2 times, most recently from 6752a82 to 6454988 Compare June 3, 2019 17:51

smortex force-pushed the floating-point-operations branch from 6454988 to b0e5659 Compare June 3, 2019 18:43

Kokan reviewed Jun 4, 2019

View reviewed changes

modules/basicfuncs/numeric-funcs.c Outdated Show resolved Hide resolved

modules/basicfuncs/numeric-funcs.c Outdated Show resolved Hide resolved

modules/basicfuncs/numeric-funcs.c Show resolved Hide resolved

modules/basicfuncs/tests/test_basicfuncs.c Show resolved Hide resolved

smortex commented Jun 4, 2019

View reviewed changes

modules/basicfuncs/numeric-funcs.c Outdated Show resolved Hide resolved

smortex force-pushed the floating-point-operations branch from 80db31e to 3ba54e8 Compare June 6, 2019 19:59

Kokan approved these changes Jun 12, 2019

View reviewed changes

bazsi approved these changes Jun 16, 2019

View reviewed changes

Kokan merged commit f3298e8 into syslog-ng:master Jun 16, 2019

smortex deleted the floating-point-operations branch June 16, 2019 20:10

Add support for floating point operations #2742

Add support for floating point operations #2742

Conversation

smortex commented May 21, 2019

kira-syslogng commented May 21, 2019

kira-syslogng commented May 21, 2019

Kokan commented May 21, 2019

kira-syslogng commented May 21, 2019

kira-syslogng commented May 21, 2019

kira-syslogng commented May 21, 2019

Kokan commented May 23, 2019

smortex commented May 23, 2019

Kokan commented May 29, 2019

A short summary of the discussion about this topic:

Re/defining the arithmetic operations:

Impact on current configurations

Storage method/1

Storage method/2

Operations error propagation

What it means for this PR:

Kokan left a comment • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

kira-syslogng commented May 31, 2019

smortex commented Jun 2, 2019

Kokan commented Jun 2, 2019

smortex commented Jun 2, 2019

smortex commented Jun 2, 2019

kira-syslogng commented Jun 2, 2019

smortex commented Jun 3, 2019

kira-syslogng commented Jun 3, 2019

kira-syslogng commented Jun 3, 2019

kira-syslogng commented Jun 3, 2019

smortex commented Jun 4, 2019

kira-syslogng commented Jun 4, 2019

kira-syslogng commented Jun 5, 2019

Kokan commented Jun 6, 2019

kira-syslogng commented Jun 6, 2019

smortex commented Jun 6, 2019

Kokan commented Jun 6, 2019

bazsi commented Jun 7, 2019 via email

Kokan commented Jun 7, 2019 • edited Loading

Kokan commented Jun 7, 2019

kira-syslogng commented Jun 7, 2019

Kokan commented Jun 12, 2019

kira-syslogng commented Jun 12, 2019

bazsi left a comment

Choose a reason for hiding this comment

Kokan left a comment •

edited

Loading

Kokan commented Jun 7, 2019 •

edited

Loading