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

[bugfix] spec compliant random-number-generator #3072

Merged
merged 4 commits into from Feb 20, 2020

Conversation

line-o
Copy link
Member

@line-o line-o commented Oct 27, 2019

Description:

  • add tests for fn:random-number-generator
  • xs:anyAtomic allowed as a seed value
  • calls to fn:random-number-generator()?next() are deterministic

Reference:

fixes #3071

Type of tests:

xqsuite

Copy link
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ERROR]   java:org.exist.xquery.XPathException exerr:ERROR cannot convert value of type xs:dateTime to Java object of type long [at line 32, column 31, source: /home/travis/build/eXist-db/exist/exist-core/src/test/xquery/xquery3/fnRandomNumberGenerator.xql]
1481In function:
1482	fn-rng:seed-dateTime() [17:5:/home/travis/build/eXist-db/exist/exist-core/src/test/xquery/xquery3/fnRandomNumberGenerator.xql]
1483	test:apply(function(*), item()*) [560:9:/home/travis/build/eXist-db/exist/exist-core/target/classes/org/exist/xquery/lib/xqsuite/xqsuite.xql]
1484	test:apply(function(*), element(), item()*) [467:9:/home/travis/build/eXist-db/exist/exist-core/target/classes/org/exist/xquery/lib/xqsuite/xqsuite.xql]
1485	test:call-test(function(*), element(), element()*) [279:32:/home/travis/build/eXist-db/exist/exist-core/target/classes/org/exist/xquery/lib/xqsuite/xqsuite.xql]. cannot convert value of type xs:dateTime to Java object of type long [at line 32, column 31]
1486[ERROR]   err:FORG0001 Invalid value for cast/constructor. can not convert 'sample seed' to xs:long [at line 20, column 27]

some type casting problem it seems

@line-o
Copy link
Member Author

line-o commented Oct 27, 2019

Yes, this is will fail until the function is implemented according to spec.

@duncdrum
Copy link
Contributor

duncdrum commented Oct 27, 2019

To merge this now i d suggest expecting the error and linking to #3071 in an inline comment, this way we would track changes in behaviour moving forward.

Unless you have a fix to the underlying java implementation ready to go, so we could add it to this PR ?

@line-o
Copy link
Member Author

line-o commented Oct 28, 2019

I would rather leave this open until the the fix is in place, @duncdrum .

@duncdrum duncdrum added the in progress Issue is actively being worked upon label Oct 28, 2019
@adamretter
Copy link
Member

@line-o happy to take a look at this with you when we meet during XML Prague 2020.

@line-o line-o force-pushed the test/fnRandomNumberGenerator branch from c47f900 to bb80848 Compare February 13, 2020 11:57
@line-o line-o changed the title test: add xqsuite for random-number-generator [bugfix] spec compliant random-number-generator Feb 13, 2020
@line-o
Copy link
Member Author

line-o commented Feb 17, 2020

@adamretter I just checked the spread of the random numbers (next() called 10000 times). The results looks OK.
The delta of each tenth is below one hundred in each segment (-100, 100).

@line-o line-o assigned wolfgangmm and unassigned wolfgangmm Feb 17, 2020
@line-o line-o requested a review from a team February 17, 2020 17:18
@adamretter
Copy link
Member

@line-o you should find that they are better than java.util.Random

Copy link
Member Author

@line-o line-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would would like to see this merged as it is.

@adamretter
Copy link
Member

adamretter commented Feb 19, 2020

@line-o sounds good, but as you and I both worked on it - you will have to find someone else to review and merge (if they are happy).

The rule is - "you never merge your own PRs"

@line-o
Copy link
Member Author

line-o commented Feb 19, 2020

I know :)

@adamretter adamretter added bug issue confirmed as bug and removed in progress Issue is actively being worked upon labels Feb 19, 2020
@adamretter adamretter added this to the eXist-5.2.1 milestone Feb 19, 2020
Copy link
Member

@joewiz joewiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-ran the one failing CI test, and it now all passes.

@joewiz joewiz merged commit edad540 into eXist-db:develop Feb 20, 2020
@joewiz
Copy link
Member

joewiz commented Feb 20, 2020

I’m curious to know, are we passing more of the qt3 tests for this function after the bug fix?

@adamretter
Copy link
Member

adamretter commented Feb 20, 2020

@joewiz with the XQTS runner after modifying build.sbt to reflect the correct eXist-db version, running:

$ java -jar exist-xqts-runner-assembly-1.0.0.jar --xqts-version HEAD --test-set fn-random-number-generator

Yields the results:

  1. 45.45% for eXist-db 5.2.0
  2. 56.82% for eXist-db 5.3.0-SNAPSHOT

Not great I know! But... in this PR I only focused on fixing the two bugs that @line-o reported to me.

@line-o
Copy link
Member Author

line-o commented Feb 20, 2020

Well these two amounted for more than 10%, I think it was worth the effort.
@adamretter could you publish the failing ones here?

@line-o line-o deleted the test/fnRandomNumberGenerator branch February 20, 2020 11:15
@adamretter
Copy link
Member

@line-o No, you can run the XQTS.

I think you will particularly groan at the fn-random-number-generator-30 failure though, as it has implications on your xBow work too!

@adamretter
Copy link
Member

See also - #3249

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issue confirmed as bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fn:random-number-generator not spec compliant
6 participants