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

[fix] unary lookups #3966

Merged
merged 5 commits into from
Aug 24, 2021
Merged

[fix] unary lookups #3966

merged 5 commits into from
Aug 24, 2021

Conversation

line-o
Copy link
Member

@line-o line-o commented Jul 5, 2021

Description:

Allow unary lookups in function calls, (anywhere) in predicates and after the simple map operator.

The XQuery grammar now

  • wraps unary lookups in a parenthesised expression that is removed in the optimisation phase
  • function argument recognises a unary lookup first before attempting to match argument placeholder (?)

Examples:

([1, 2], ["a", "b"]) ! string-join(?*, "|")
(map{1: 1}, map{1: 3}, map{1: 2})[1 > ?1] ! ?1

Additional change: correct the syntax of a dumped map expression to use current syntax.
For the second example that produces:

DEBUG (XQuery.java [compile]:143) - Query diagnostics:
(
    map {1 : 1}, map {1 : 3}, map {1 : 2}
) [1 > ?1] ! ?1

Reference:

fixes #1655
fixes #3491

Relevant XQuery 3.1 specification: 3.11.3.1 Unary Lookup

Type of tests:

Added and uncommented XQSuite tests

@line-o
Copy link
Member Author

line-o commented Jul 5, 2021

With this PR applied running XQTS results in

Tests Failures Errors Skipped Success rate Time
31557 5152 554 1260 81.92% 1383.580

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 5, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@adamretter
Copy link
Contributor

@line-o Out of interest, are these results from the W3C fixed XQTS or the living one on GitHub?

@line-o
Copy link
Member Author

line-o commented Jul 5, 2021

@adamretter the above results are from a run against the HEAD of XQTS (-x HEAD)

@adamretter
Copy link
Contributor

@line-o Before your change, for 5.3.0 release I see:

Tests Failures Errors Skipped Success Rate Time
31738 5265 555 1298 81.66% 647.980

Did you refresh your version of XQTS from GitHub? I ask because we have different skipped counts?

@joewiz
Copy link
Member

joewiz commented Jul 5, 2021

To ensure the latest xqts was downloaded afresh, I ran git clean -xdf in my clone of exist-xqts-runner master (and confirmed that the work folder was deleted), performed sbt clean assembly and target/scala-2.13/exist-xqts-runner-assembly-1.0.0.jar -x HEAD (following eXist-db/exist-xqts-runner#17), and got the same number for "Tests" and "Skipped" as @line-o.

Tests Failures Errors Skipped Success rate Time
31557 5167 551 1260 81.88% 439.974

Comparing these 5.3.0 results to @line-o's results for this PR, we can see that with this PR, eXist passes 15 more tests than before, has 3 fewer errors, and its success rate has improved by .04%.

@line-o
Copy link
Member Author

line-o commented Jul 5, 2021

Interesting since I saw 31700+ Tests on one run but then it went down to 31557 on the subsequent run. I have no idea what could cause this.

@joewiz the total amount of tests went down to 31557 after sbt clean in my case

@line-o line-o added this to the eXist-5.3.1 milestone Jul 5, 2021
@adamretter
Copy link
Contributor

To ensure the latest xqts was downloaded afresh, I ran git clean -xdf

Okay, that's interesting, pretty sure I did the same, let me try again..

@joewiz Also just to confirm, which version of eXist-db is set in your build.sbt file?

@line-o
Copy link
Member Author

line-o commented Jul 5, 2021

I did change the version to 5.4.0-SNAPSHOT and also did a mvn clean install in the working directory where I checked out this branch.

@joewiz
Copy link
Member

joewiz commented Jul 5, 2021

@adamretter In my build.sbt, the eXist version is set to 5.3.0, as in https://github.com/eXist-db/exist-xqts-runner/blob/a47a6b4abec2d00bf0b3eae85032fd2599dd5664/build.sbt#L17 (current master HEAD). (My purpose was to produce the results for 5.3.0, for comparison to @line-o's results for this PR.)

@joewiz
Copy link
Member

joewiz commented Jul 5, 2021

@line-o I can confirm that this PR fixes the issue reported in #1655. In other words:

xquery version "3.1";

let $map := map { "name": "mike" }
return
    $map["mike" eq ?name]

... now returns map { "name": "mike" }.

@line-o
Copy link
Member Author

line-o commented Jul 5, 2021

I had moved the first test result in a separate folder and had a look

Tests Failures Errors Skipped Success rate Time
31738 5261 558 1298 81.67% 1294.932

@adamretter
Copy link
Contributor

adamretter commented Jul 5, 2021

My purpose was to produce the results for 5.3.0

@joewiz Likewise :-)

So I ran it again, I now at least get the same number of tests; I must have inadvertently had some old data before - but I can't see how!

I am on the HEAD of master of XQTS, and ran with -x HEAD.
Strangely though, I get less failures than yourself!?!
I am on macOS 11.2.3 and Zulu JDK 8. Not that it should make a difference.

My yield is (with build.sbt indicating eXist-db 5.3.0):

Tests Failures Errors Skipped Success rate Time
31557 5156 551 1260 81.92% 1173.726

@joewiz joewiz requested a review from a team July 26, 2021 16:21
@joewiz
Copy link
Member

joewiz commented Jul 26, 2021

@dizzzz Could you please restart Appveyor when you have the chance?

@adamretter
Copy link
Contributor

With this PR applied running XQTS results in

Can you show before and after figures please?

@joewiz
Copy link
Member

joewiz commented Aug 1, 2021

Besides running XQTS on the PR and its parent commit, I wrote a query that tells us which test cases actually changed. I'll share XQTS aggregate results, then dig into the specific changes. TL/DR: The PR causes 4 tests that previously failed to pass and 3 tests that previously failed to error.

XQTS aggregate results

I ran exist-xqts-runner 3 times for each build, using the methodology developed in eXist-db/exist-xqts-runner#19. I am on macOS 11.5.1 with OpenJDK 1.8.0_302 (liberica-jdk8-full) and sbt 1.5.5.

Baseline

With the eXist-5.3.0 tag:

Tests Failures Errors Skipped Success rate Time
31557 5167 551 1260 81.88% 370.673
31557 5167 551 1260 81.88% 482.265
31557 5167 551 1260 81.88% 543.269

With the commit just before this PR (d064fc4):

Tests Failures Errors Skipped Success rate Time
31557 5166 551 1260 81.88% 393.969
31557 5167 551 1260 81.88% 398.634
31557 5166 551 1260 81.88% 415.548

So the baseline is pretty clear from the above. We have +/- 1 failure in 5.3.0 and the 5.4.0-SNAPSHOT commit that was this PR's parent.

This PR

Here are the results with this PR (checking out this PR's branch):

Tests Failures Errors Skipped Success rate Time
31557 5162 554 1260 81.89% 379.088
31557 5159 554 1260 81.90% 496.157
31557 5160 554 1260 81.89% 490.504

All the aggregate results tell us is that:

  • overall, failures are down by 5-8.
  • overall, errors are up by 3.
  • overall, success rate is up by .01-.02%.

Missing is which tests previous failed and are now passing, which results are now generating errors, and what the tests now producing errors were returning before.

Analysis of changes in test results

To meaningfully analyze the test results, we have to compare which test results changed and what the actual changes were. Here's what appears to be the actual changes between the PR's parent and the PR:

testcase name d064fc4 this PR
UnaryLookup-011 fail error
UnaryLookup-015 fail pass
UnaryLookup-016 fail error
UnaryLookup-017 fail error
UnaryLookup-023 fail pass
UnaryLookup-025 fail pass
UnaryLookup-046 fail pass

The 4 cases of fails that are now pass are unambiguous improvements. Thus, I won't dig into them - but they are clearly a major portion of the PR's contribution.

Let's examine the 3 cases where failures turned into errors:

UnaryLookup-011

Test:

   <test-case name="UnaryLookup-011">
      <description>Unary lookup with no context item</description>
      <created by="Michael Kay" on="2014-11-27"/>
      <dependency type="spec" value="XP31+ XQ31+"/>
      <dependency type="feature" value="higherOrderFunctions"/>
      <test>let $d := function($x) {$x + ?2} return $d(12)</test>
      <result>
         <error code="XPDY0002"/>
      </result>
   </test-case>

Before:

Expected: 'AssertDeepEquals('a', 'b', 'e')', but query returned an error: QueryError(ERROR,exerr:ERROR <AST>:0:0: unexpected end of subtree [at line 1, column 55])

After:

java.lang.NullPointerException

Stacktrace:

Caused by: java.lang.NullPointerException
	at org.exist.xquery.Lookup.eval(Lookup.java:83)
	at org.exist.xquery.AbstractExpression.eval(AbstractExpression.java:71)
	at org.exist.xquery.PathExpr.eval(PathExpr.java:279)
	at org.exist.xquery.AbstractExpression.eval(AbstractExpression.java:71)
	at org.exist.xquery.PathExpr.eval(PathExpr.java:279)
	at org.exist.xquery.OpNumeric.eval(OpNumeric.java:115)
	at org.exist.xquery.UserDefinedFunction.eval(UserDefinedFunction.java:161)
	at org.exist.xquery.FunctionCall.evalFunction(FunctionCall.java:276)
	at org.exist.xquery.FunctionCall.eval(FunctionCall.java:194)
	at org.exist.xquery.AbstractExpression.eval(AbstractExpression.java:71)
	at org.exist.xquery.value.FunctionReference.eval(FunctionReference.java:86)
	at org.exist.xquery.DynamicFunctionCall.eval(DynamicFunctionCall.java:97)
	at org.exist.xquery.DebuggableExpression.eval(DebuggableExpression.java:58)
	at org.exist.xquery.LetExpr.eval(LetExpr.java:110)
	at org.exist.xquery.AbstractExpression.eval(AbstractExpression.java:71)
	at org.exist.xquery.PathExpr.eval(PathExpr.java:279)
	at org.exist.xquery.AbstractExpression.eval(AbstractExpression.java:71)
	at org.exist.xquery.XQuery.execute(XQuery.java:261)
	at org.exist.xquery.XQuery.execute(XQuery.java:185)
	at org.exist.xquery.XQuery.execute(XQuery.java:180)
	at org.exist.xqts.runner.ExistConnection.$anonfun$executeQuery$36(ExistServer.scala:306)

UnaryLookup-016

Test:

   <test-case name="UnaryLookup-016">
      <description>No syntactic confusion with place-markers</description>
      <created by="Michael Kay" on="2014-11-27"/>
      <dependency type="spec" value="XP31+ XQ31+"/>
      <dependency type="feature" value="higherOrderFunctions"/>
      <test>(['a', 'b', 'c'], ['b', 'c', 'd'], ['e', 'f', 'b'])[contains(?1, ?, 'http://www.w3.org/2005/xpath-functions/collation/codepoint')('a')]</test>
      <result>
         <assert-deep-eq>['a', 'b', 'c']</assert-deep-eq>
      </result>
   </test-case>

Before:

Expected: 'AssertDeepEquals(['a', 'b', 'c'])', but query returned an error: QueryError(ERROR,exerr:ERROR org.exist.xquery.XPathException: err:XPST0003 expecting closing parenthesis ')', found '1' [at line 1, column 63] )

After:

java.lang.ClassCastException: org.exist.xquery.Function$Placeholder cannot be cast to org.exist.xquery.PathExpr

Stacktrace:

Caused by: java.lang.ClassCastException: org.exist.xquery.Function$Placeholder cannot be cast to org.exist.xquery.PathExpr
	at org.exist.xquery.FunctionFactory.contains(FunctionFactory.java:191)
	at org.exist.xquery.FunctionFactory.createFunction(FunctionFactory.java:94)
	at org.exist.xquery.FunctionFactory.createFunction(FunctionFactory.java:61)
	at org.exist.xquery.FunctionFactory.createFunction(FunctionFactory.java:57)
	at org.exist.xquery.parser.XQueryTreeParser.functionCall(XQueryTreeParser.java:11206)
	at org.exist.xquery.parser.XQueryTreeParser.primaryExpr(XQueryTreeParser.java:7787)
	at org.exist.xquery.parser.XQueryTreeParser.expr(XQueryTreeParser.java:3568)
	at org.exist.xquery.parser.XQueryTreeParser.postfixExpr(XQueryTreeParser.java:10758)
	at org.exist.xquery.parser.XQueryTreeParser.primaryExpr(XQueryTreeParser.java:7748)
	at org.exist.xquery.parser.XQueryTreeParser.expr(XQueryTreeParser.java:3568)
	at org.exist.xquery.parser.XQueryTreeParser.mainModule(XQueryTreeParser.java:3993)
	at org.exist.xquery.parser.XQueryTreeParser.module(XQueryTreeParser.java:3936)
	at org.exist.xquery.parser.XQueryTreeParser.xpath(XQueryTreeParser.java:3591)
	at org.exist.xquery.XQuery.compile(XQuery.java:128)
	at org.exist.xquery.XQuery.compile(XQuery.java:79)
	at org.exist.xquery.XQuery.compile(XQuery.java:71)
	at org.exist.xqts.runner.ExistConnection.getCompiledQuery$1(ExistServer.scala:256)

UnaryLookup-017

Test:

   <test-case name="UnaryLookup-017">
      <description>No syntactic confusion with place-markers</description>
      <created by="Michael Kay" on="2014-11-27"/>
      <dependency type="spec" value="XP31+ XQ31+"/>
      <dependency type="feature" value="higherOrderFunctions"/>
      <test>(['a', 'b', 'c'], ['b', 'c', 'd'], ['e', 'f', 'b'])[contains(?1, ?)('a')]</test>
      <result>
         <assert-deep-eq>['a', 'b', 'c']</assert-deep-eq>
      </result>
   </test-case>

Before:

Expected: 'AssertDeepEquals(['a', 'b', 'c'])', but query returned an error: QueryError(ERROR,exerr:ERROR org.exist.xquery.XPathException: err:XPST0003 expecting closing parenthesis ')', found '1' [at line 1, column 63] )" type="junit.framework.AssertionFailedError

After:

java.lang.ClassCastException: org.exist.xquery.Function$Placeholder cannot be cast to org.exist.xquery.PathExpr

Stacktrace:

Caused by: java.lang.ClassCastException: org.exist.xquery.Function$Placeholder cannot be cast to org.exist.xquery.PathExpr
	at org.exist.xquery.FunctionFactory.contains(FunctionFactory.java:191)
	at org.exist.xquery.FunctionFactory.createFunction(FunctionFactory.java:94)
	at org.exist.xquery.FunctionFactory.createFunction(FunctionFactory.java:61)
	at org.exist.xquery.FunctionFactory.createFunction(FunctionFactory.java:57)
	at org.exist.xquery.parser.XQueryTreeParser.functionCall(XQueryTreeParser.java:11206)
	at org.exist.xquery.parser.XQueryTreeParser.primaryExpr(XQueryTreeParser.java:7787)
	at org.exist.xquery.parser.XQueryTreeParser.expr(XQueryTreeParser.java:3568)
	at org.exist.xquery.parser.XQueryTreeParser.postfixExpr(XQueryTreeParser.java:10758)
	at org.exist.xquery.parser.XQueryTreeParser.primaryExpr(XQueryTreeParser.java:7748)
	at org.exist.xquery.parser.XQueryTreeParser.expr(XQueryTreeParser.java:3568)
	at org.exist.xquery.parser.XQueryTreeParser.mainModule(XQueryTreeParser.java:3993)
	at org.exist.xquery.parser.XQueryTreeParser.module(XQueryTreeParser.java:3936)
	at org.exist.xquery.parser.XQueryTreeParser.xpath(XQueryTreeParser.java:3591)
	at org.exist.xquery.XQuery.compile(XQuery.java:128)
	at org.exist.xquery.XQuery.compile(XQuery.java:79)
	at org.exist.xquery.XQuery.compile(XQuery.java:71)
	at org.exist.xqts.runner.ExistConnection.getCompiledQuery$1(ExistServer.scala:256)

I hope this information helps in evaluating the PR.

Copy link
Member

@dizzzz dizzzz left a comment

Choose a reason for hiding this comment

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

Unable to review the .g file.

Copy link
Member

@dizzzz dizzzz left a comment

Choose a reason for hiding this comment

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

We should get rid of the traces

@line-o
Copy link
Member Author

line-o commented Aug 2, 2021

What do you mean by "traces" @dizzzz ?

@line-o
Copy link
Member Author

line-o commented Aug 2, 2021

@joewiz This is a very thorough research and we are finally able to compare XQTS test results in detail. I cannot thank you enough for taking on this task.
The results speak for themselves. I am curious why this PR was not merged.
The (new) errors that are know thrown are the result of a separate issue.

@line-o
Copy link
Member Author

line-o commented Aug 2, 2021

Related issue: Placeholder after arrow function #3336
The above is the reason for the new failures

@dizzzz
Copy link
Member

dizzzz commented Aug 4, 2021

In the monday call we have talked about the PR; we really want it pulled it, but these 3 stacktraces are new and should be addressed. 2 out of 3 stacktraces are probably straightforward to fix. In all the PR is a significant improvement!

@line-o
Copy link
Member Author

line-o commented Aug 4, 2021

As I laid out, now that the test can be parsed a different issue surfaces. That is specifically the parser's inability to recognize the argument placeholder "?" In certain cases (see #3336).

I strongly disagree with this blocking the merge, as both issues are unrelated.

@line-o
Copy link
Member Author

line-o commented Aug 4, 2021

Compare "UnaryLookup-017" slightly rewritten to remove the argument placeholder:

(['a', 'b', 'c'], ['b', 'c', 'd'], ['e', 'f', 'b'])[contains(?1, "a")]

@line-o
Copy link
Member Author

line-o commented Aug 4, 2021

One could argue that "UnaryLookup-011" should be catched and return the correct error code with location information.

@line-o
Copy link
Member Author

line-o commented Aug 5, 2021

I will provide a test and a fix for unary lookups, when the context item is absent.
As UnaryLookup-011 shows that this leads to an NPE.

@joewiz
Copy link
Member

joewiz commented Aug 5, 2021

@line-o Am I correct in this summary of your response to the xqts findings described above and summarized by @dizzzz?

  1. UnaryLookup-011 (NPE): You'll submit a commit addressing the NPE.
  2. UnaryLookup-016 and UnaryLookup-017 (ClassCastException): These errors are not introduced by or within the scope of this PR. Rather, this PR helps eXist expose the flaws described in [BUG] argument placeholder after arrow operator not spec compliant #3336. Thus, the error messages thrown are in fact an improvement on the previous behavior, because it directly sheds light on the issue in [BUG] argument placeholder after arrow operator not spec compliant #3336 rather than obscuring it the previous failure. You will not submit a fix to this issue in this PR. Rather, it should be addressed in [BUG] argument placeholder after arrow operator not spec compliant #3336.

So once your fix to the NPE is fixed, the PR will be ready for a final review and merge, from your perspective?

@line-o
Copy link
Member Author

line-o commented Aug 6, 2021

@joewiz Yes, that is exactly what I wanted to say :)

line-o added 5 commits August 17, 2021 19:25
The wrapping parenthesized expression will be removed in the optimisation step.
It allows a set of operations that are valid XQuery but were not parsed properly before.

Examples:

- `[1] ! ?1`
- `map{1:1} ! ?*`
- `(map{1:1}, map{1:2})[1 < ?1]`
fixes eXist-db#3491

With this PR applied the XQuery grammar tries to match exprSingle first.
Unary lookups are therefore treated as exprSingle instead of the `?` being
interpreted as an argument placeholder.

Examples:

- `([1], [1,2]) ! sum(?*)`
- `map{1:1} ! count(?*)`
Map expressions use currentt map constructor syntax.
`map { $key : $value }`
The missing contextSequence resulted in an NPE.
In Lookup this will now be checked first and the correct Error XPDY0002 will be thrown instead.

- Add testcase to mapsLookup.xql
@line-o line-o force-pushed the fix/unary-lookups branch from 3215564 to 5f8feb0 Compare August 17, 2021 17:25
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@line-o line-o requested a review from dizzzz August 17, 2021 18:18
@line-o
Copy link
Member Author

line-o commented Aug 17, 2021

I am confident that with the commit 5f8feb0 UnaryLookup-011 will pass.

@line-o
Copy link
Member Author

line-o commented Aug 17, 2021

XQTS test run results

Tests Failures Errors Skipped Success rate Time
31557 5141 553 1260 81.96% 1521.656

@joewiz
Copy link
Member

joewiz commented Aug 22, 2021

By comparing the actual xqts results using the query above, we can not only be confident - we can be absolutely certain. The comparison query also reveals 4 other improvements.

testsuite package testsuite name testcase name this PR before the latest commits current state of PR
XQTS_HEAD fn-function-lookup fn-function-lookup-823 failure pass
XQTS_HEAD fn-function-lookup fn-function-lookup-824 failure pass
XQTS_HEAD op-same-key same-key-022 failure pass
XQTS_HEAD op-same-key same-key-024 failure pass
XQTS_HEAD prod-UnaryLookup UnaryLookup-011 error pass

These results were consistent across 3 runs of the test runner.

Great job, @line-o!

Copy link
Member

@dizzzz dizzzz left a comment

Choose a reason for hiding this comment

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

LGTM! Näyttää hyvältä!

@dizzzz dizzzz merged commit 3582600 into eXist-db:develop Aug 24, 2021
@line-o line-o deleted the fix/unary-lookups branch November 26, 2021 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants