Convert in-memory Column.round to Java

[GregoryTravis]

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

• The documentation has been updated, if necessary.
• Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
• All code follows the
  Scala,
  Java,
  and
  Rust
  style guides. In case you are using a language not listed above, follow the Rust style guide.
• All code has been tested:
  • Unit tests have been written where possible.
  • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

[radeusgd]

Looks good overall, I think the direction is good.

I'm glad that the refactor splitting into unary and binary ops actually made it a bit cleaner to add a ternary op as you just did.

I think the direction is good to proceed, just needs polishing. In particular, please ensure that the result type is like we expect.

[GregoryTravis]

Benchmarks: significant improvement for floats; not much for ints:

float old round decimal_places=0 use_bankers=True average: 374.8ms
float new round decimal_places=0 use_bankers=True average: 21.4ms
float old round decimal_places=-2 use_bankers=True average: 266.25ms
float new round decimal_places=-2 use_bankers=True average: 16.02ms
float old round decimal_places=2 use_bankers=True average: 259.43ms
float new round decimal_places=2 use_bankers=True average: 19.17ms
float old round decimal_places=0 use_bankers=False average: 258.15ms
float new round decimal_places=0 use_bankers=False average: 18.69ms
float old round decimal_places=-2 use_bankers=False average: 211.43ms
float new round decimal_places=-2 use_bankers=False average: 19.06ms
float old round decimal_places=2 use_bankers=False average: 205.5ms
float new round decimal_places=2 use_bankers=False average: 15.09ms

int old round decimal_places=0 use_bankers=True average: 0.28ms
int new round decimal_places=0 use_bankers=True average: 0.26ms
int old round decimal_places=-2 use_bankers=True average: 0.1ms
int new round decimal_places=-2 use_bankers=True average: 0.09ms
int old round decimal_places=2 use_bankers=True average: 0.05ms
int new round decimal_places=2 use_bankers=True average: 0.06ms
int old round decimal_places=0 use_bankers=False average: 0.12ms
int new round decimal_places=0 use_bankers=False average: 0.07ms
int old round decimal_places=-2 use_bankers=False average: 0.04ms
int new round decimal_places=-2 use_bankers=False average: 0.05ms
int old round decimal_places=2 use_bankers=False average: 0.03ms
int new round decimal_places=2 use_bankers=False average: 0.05ms

[GregoryTravis]

@radeusgd value_type tests are already in Column_Spec.

[GregoryTravis]

New benchmarks; indeed, I commented out the parameters and forgot I had done it. The benchmarks show improvements for both types. (Except in the fast-return case, where it isn't doing anything anyway.)

.round floats
old float round decimal_places=0 use_bankers=True average: 368.67ms
new float round decimal_places=0 use_bankers=True average: 23.86ms
.round ints
old int round decimal_places=0 use_bankers=True average: 0.33ms
new int round decimal_places=0 use_bankers=True average: 0.34ms
.round floats
old float round decimal_places=-2 use_bankers=True average: 287.51ms
new float round decimal_places=-2 use_bankers=True average: 19.88ms
.round ints
old int round decimal_places=-2 use_bankers=True average: 313.95ms
new int round decimal_places=-2 use_bankers=True average: 14.59ms
.round floats
old float round decimal_places=2 use_bankers=True average: 239.04ms
new float round decimal_places=2 use_bankers=True average: 18.98ms
.round ints
old int round decimal_places=2 use_bankers=True average: 0.06ms
new int round decimal_places=2 use_bankers=True average: 0.07ms
.round floats
old float round decimal_places=0 use_bankers=False average: 233.73ms
new float round decimal_places=0 use_bankers=False average: 21.27ms
.round ints
old int round decimal_places=0 use_bankers=False average: 0.11ms
new int round decimal_places=0 use_bankers=False average: 0.12ms
.round floats
old float round decimal_places=-2 use_bankers=False average: 210.69ms
new float round decimal_places=-2 use_bankers=False average: 21.3ms
.round ints
old int round decimal_places=-2 use_bankers=False average: 274.66ms
new int round decimal_places=-2 use_bankers=False average: 28.59ms
.round floats
old float round decimal_places=2 use_bankers=False average: 295.04ms
new float round decimal_places=2 use_bankers=False average: 19.86ms
.round ints
old int round decimal_places=2 use_bankers=False average: 0.04ms
new int round decimal_places=2 use_bankers=False average: 0.03ms

[jdunkerley]

should we leave this at 100? or is it so long now we want it shorter!

[GregoryTravis]

It takes 47s with iter_size = 10 so I assume we don't want it much longer than that.

[radeusgd]

For future - I think in that case we may want to consider to reduce the vector_size instead, to make each iteration a bit shorter - but to keep the number of iterations. 10 may be too few to see warmup and also to see the distribution of the timings (i.e. if the variance is low or high).

[JaroslavTulach]

I am interested in ensuring consistency between the builtin and the column implementation. Either by sharing the algorithm or by sharing the tests. Otherwise OK as far as I can say.

[JaroslavTulach]

Is this a copy of the code in RoundNode? Shall not we seek a way to share the code among both versions?

[GregoryTravis]

After discussing it, we decided it was sufficient to share the tests; sharing the implementation between builtins and regular polyglot libraries is a bigger task.

[JaroslavTulach]

I like the naming - I.

[GregoryTravis]

Thank @radeusgd for that.

[JaroslavTulach]

Out of curiosity: shall not the exception be NullPointerException? Or use java.util.Objects.requireNonNull...

[GregoryTravis]

Removed the null check.

[JaroslavTulach]

This exception would be thrown if decimalPlacesObject was null. E.g. do we need the above == null check at all?

[GregoryTravis]

Removed the null check.

[JaroslavTulach]

To ensure the behavior of RoundNode builtin and the Column.round is the same, can we generalize this Numbers_Spec and run it twice on the same values? Once using regular round builtin, once creating a Column with the same value and rounding via Column.round? I assume the result should be the same.

[GregoryTravis]

Done.

[JaroslavTulach]

Sharing the tests this way shall ensure the two rounding implementations are as identical as we need.

[JaroslavTulach]

+1

[JaroslavTulach]

Yup. do_op creates a table with row "x" and single value n and then applies the operation to column "x" and returns the result.

[radeusgd]

Is this import used?

[radeusgd]

I don't think this is the right place to have this.

Standard.Test is a test library, like JUnit. We should not distribute our tests with it.

IMO we should just move this test altogether to Table_Tests if we want to run it for both scalars and Columns. I don't think there is much harm in moving the scalar case from Tests to Table_Tests - IMO much less than distributing our internal test suites as part of a general and user-facing testing library.