odd negative sign in modulo results

[kelvintaywl]

Hi!

firstly, thank you so much for this tool.
It's been amazingly helpful for me 🙇 .

I wanted to file an issue about seeing negative values in the modulo (%) result.

To reproduce:

$ csvq -v
csvq version 1.18.1

$ csvq "SELECT (CEIL(13041 / 60) % 60)"
+-------------------------+
| (CEIL(13041 / 60) % 60) |
+-------------------------+
|                     -23 |
+-------------------------+

I was expecting 37 there since CEIL(13041/60) = 217 and 217 % 60 = 37, not -23.

I am thinking this is a possible bug 🤔

[mithrandie]

It is the result of a modulo calculation on the floating-point numbers, since the result of the CEIL function is a float. However, it may be a bad idea to apply the % operator to float.

As a workaround, you can avoid that result by explicitly converting the ceil result to integer.

$ csvq "SELECT INTEGER(CEIL(13041/60))%60"

[kpym]

@mithrandie I don't get it, even for floats a and b the result a % b should be in the range [0,|b|), right?

If you use math.Mod behind the scenes, the result could be negative if a is negative. But:

• there CEIL(13041 / 60) is not negative;
• the go behavior of math.Mod is not "standard". If you decide to stick with it, you should probably document it. Otherwise, you can correct the result of a/b by simply adding |b| if it is negative.

[kpym]

After verification, I'm wrong about "standard". There is no such thing about the sign of modulo. In some languages (like go and javascript) the sign of a/b is the same as a, and in other languages (like python) it is the same as b. And in math it is often fixed as positive.

But in any case, if a and b are positive (like in CEIL(13041 / 60) % 60), the modulo must be positive.

And in any case, it will be good to document the behavior of % with respect to the sign.

[kpym]

@mithrandie You use math.Remainder instead of math.Mod. This is why you get a negative remainder from two positive numbers. Is this intentional? Isn't it better to use math.Mod instead?

[mithrandie]

Ah, I didn't know there was the math.Mod function...
That would be more appropriate for the % operator. Thank you.

[kelvintaywl]

hi @mithrandie and @kpym

Thanks so much for the insights!
I am nowhere familiar with the implementation nor Go, so your details here helped me understand better.

For myself, i am not blocked since we are using it for analysis internally; no huge repercussions with the negative sign.

I will give the suggestion here a try too.

I'll leave this open since it'd be nice to close this if a patch is on the way 🤓
thanks so much, again! 🙇

[kelvintaywl]

hi @mithrandie
I wanted to share that I was able to fix/work-around this with casting the ceil result with INTEGER.
Thank you so much!

I've leave it to you, if you'd like to keep this open, given #109 (comment)