search

jOOQ / jOOL

jOOλ - The Missing Parts in Java 8 jOOλ improves the JDK libraries in areas where the Expert Group's focus was elsewhere. It adds tuple support, function support, and a lot of additional functionality around sequential Streams. The JDK 8's main efforts (default methods, lambdas, and the Stream API) were focused around maintaining backwards compatibility and implementing a functional API for parallelism.
http://www.jooq.org/products
Apache License 2.0
2.08k stars 167 forks source link

Wrong result for Agg.regrR2Double() #397

Closed magicprinc closed 1 year ago

magicprinc commented 1 year ago

This is not something I have made. I checked out last sources from jOOQ/jOOL repo and ran maven: The test has failed.

[ERROR] Failures:
[ERROR]   CollectorTests.testRegrR2:1472 expected:<Optional[1.0]> but was:<Optional.empty>`

[ERROR] Tests run: 52, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.114 s <<< FAILURE! - in org.jooq.lambda.CollectorTests
[ERROR] org.jooq.lambda.CollectorTests.testRegrR2  Time elapsed: 0.004 s  <<< FAILURE!
java.lang.AssertionError: expected:<Optional[1.0]> but was:<Optional.empty>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.junit.Assert.assertEquals(Assert.java:146)
        at org.jooq.jool/org.jooq.lambda.CollectorTests.testRegrR2(CollectorTests.java:1472)

I don't use it, so I don't dare to touch it.

lukaseder commented 1 year ago

Thanks, can confirm. Will look into this soon

lukaseder commented 1 year ago

The test is correct. The expected result is 1.0:

select regr_r2(x, y)
from (values (1, 1), (1, 2)) t (x, y)

Produces:

|regr_r2|
|-------|
|1      |
lukaseder commented 1 year ago

https://github.com/jOOQ/jOOQ/issues/11547 defines REGR_R2 as:

REGR_R2(x, y) = CASE 
  WHEN VAR_POP(NVL2(x, y, NULL)) = 0 THEN NULL 
  WHEN VAR_POP(NVL2(y, x, NULL)) = 0 THEN 1 
  ELSE CORR(x, y)^2 
END
lukaseder commented 1 year ago

The formula is implemented correctly, but the internal mapAll(Tuple3<Optional<T1>, Optional<T2>, Optional<T3>>) isn't. It's requires all optionals to be non-empty in order to map anything, when in fact this isn't what the CASE expression suggests.

lukaseder commented 1 year ago

Well, mapAll() is implemented correctly, but using it is wrong.

lukaseder commented 1 year ago

The reason why this hasn't been detected in our CI is because we recently migrated CI and jOOλ wasn't part of the new installation...