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

GH-2078 property path negation and inversion #2080

Merged
merged 4 commits into from
Apr 10, 2020

Conversation

hmottestad
Copy link
Contributor

GitHub issue resolved: #2078

Briefly describe the changes proposed in this PR:


PR Author Checklist:

  • my pull request is self-contained
  • I've added tests for the changes I made
  • every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change
  • every commit has been signed off

Note: we merge all feature pull requests using squash and merge. See RDF4J git merge strategy for more details.

@hmottestad hmottestad mentioned this pull request Apr 8, 2020
@hmottestad hmottestad added the WIP label Apr 8, 2020
@abrokenjester abrokenjester changed the base branch from develop to master April 9, 2020 02:39
@abrokenjester abrokenjester changed the base branch from master to develop April 9, 2020 02:39
@abrokenjester abrokenjester force-pushed the GH-2078-property-path-negation-and-inversion branch from 8adb315 to de8f9d8 Compare April 9, 2020 02:41
@abrokenjester
Copy link
Contributor

abrokenjester commented Apr 9, 2020

I've pushed what I think is a decent fix for the immediate issue.

@hmottestad this is really a bug fix and should be merged to master, but unfortunately the feature branch is based off of develop. Could you rebase and target master? Alternatively, we can merge this and then cherry pick back onto master.

@abrokenjester
Copy link
Contributor

ACtually just realized i didn't run all compliance tests - I think I may have overlooked a corner case.

@abrokenjester abrokenjester linked an issue Apr 9, 2020 that may be closed by this pull request
@abrokenjester abrokenjester changed the title Gh 2078 property path negation and inversion GH-2078 property path negation and inversion Apr 9, 2020
Copy link
Contributor

@abrokenjester abrokenjester left a comment

Choose a reason for hiding this comment

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

I'm fine with merging this as-is. A squash-merge + cherry-pick to get it onto master is fine with me. I'm on the fence whether we should log a separate issue to refactor the SPARQL parser a bit, make it less of a tangle.

@hmottestad
Copy link
Contributor Author

hmottestad commented Apr 9, 2020

Thank for fixing this @jeenbroekstra

I've added a couple more tests, and one of them looks a bit wonky. It's not so much incorrect I think as it is just a bit bloated.

This is the query:

PREFIX : <http://example.org/>
ASK { :IBM ^(:|!:) ?jane, ?jane2. }

And this is the plan:

Slice ( limit=1 )
   Join
      Union
         Join
            StatementPattern (resultSizeEstimate=UNKNOWN)
               Var (name=jane)
               Var (name=_const_7193c6e1_uri, value=http://example.org/, anonymous)
               Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)
            StatementPattern (resultSizeEstimate=UNKNOWN)
               Var (name=jane2)
               Var (name=_const_7193c6e1_uri, value=http://example.org/, anonymous)
               Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)
         Filter
            Compare (!=)
               Var (name=_anon_ee21cdb5_6a14_4539_a854_ed672dcc589d, anonymous)
               ValueConstant (value=http://example.org/)
            Join
               StatementPattern (resultSizeEstimate=UNKNOWN)
                  Var (name=jane2)
                  Var (name=_anon_ee21cdb5_6a14_4539_a854_ed672dcc589d, anonymous)
                  Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)
               StatementPattern (resultSizeEstimate=UNKNOWN)
                  Var (name=jane)
                  Var (name=_anon_ee21cdb5_6a14_4539_a854_ed672dcc589d, anonymous)
                  Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)
      Union
         Join
            StatementPattern (resultSizeEstimate=UNKNOWN)
               Var (name=jane)
               Var (name=_const_7193c6e1_uri, value=http://example.org/, anonymous)
               Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)
            StatementPattern (resultSizeEstimate=UNKNOWN)
               Var (name=jane2)
               Var (name=_const_7193c6e1_uri, value=http://example.org/, anonymous)
               Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)
         Filter
            Compare (!=)
               Var (name=_anon_ee21cdb5_6a14_4539_a854_ed672dcc589d, anonymous)
               ValueConstant (value=http://example.org/)
            Join
               StatementPattern (resultSizeEstimate=UNKNOWN)
                  Var (name=jane2)
                  Var (name=_anon_ee21cdb5_6a14_4539_a854_ed672dcc589d, anonymous)
                  Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)
               StatementPattern (resultSizeEstimate=UNKNOWN)
                  Var (name=jane)
                  Var (name=_anon_ee21cdb5_6a14_4539_a854_ed672dcc589d, anonymous)
                  Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)

Should be something like this:

Slice ( limit=1 )
   Join
      Union
         StatementPattern (resultSizeEstimate=UNKNOWN)
            Var (name=jane)
            Var (name=_const_7193c6e1_uri, value=http://example.org/, anonymous)
            Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)
         Filter
            Compare (!=)
               Var (name=_anon_8b0343a3_70a5_47db_adde_8128fbfa9f0d, anonymous)
               ValueConstant (value=http://example.org/)
            StatementPattern (resultSizeEstimate=UNKNOWN)
               Var (name=jane)
               Var (name=_anon_8b0343a3_70a5_47db_adde_8128fbfa9f0d, anonymous)
               Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)
      Union
         StatementPattern (resultSizeEstimate=UNKNOWN)
            Var (name=jane2)
            Var (name=_const_7193c6e1_uri, value=http://example.org/, anonymous)
            Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)
         Filter
            Compare (!=)
               Var (name=_anon_753de35b_1560_4eca_a727_9b48d975b9fb, anonymous)
               ValueConstant (value=http://example.org/)
            StatementPattern (resultSizeEstimate=UNKNOWN)
               Var (name=jane2)
               Var (name=_anon_753de35b_1560_4eca_a727_9b48d975b9fb, anonymous)
               Var (name=_const_1bf5dbd3_uri, value=http://example.org/IBM, anonymous)

@abrokenjester
Copy link
Contributor

abrokenjester commented Apr 10, 2020

Yes, I did notice that too, and it is a bug of some sort (the produced algebra is not incorrect, just more complex than it needs to be - it essentially duplicate the entire tree for both object list arguments). It is not directly related to this particular bug though. I suggest you remove that test again and log a separate issue.

@abrokenjester abrokenjester marked this pull request as draft April 10, 2020 00:04
@abrokenjester abrokenjester removed the WIP label Apr 10, 2020
@abrokenjester
Copy link
Contributor

Huh, look at that, new feature in github I think: you can convert a pull request back to draft (option appeared for me top-right, underneath the list of reviewers).

@hmottestad hmottestad force-pushed the GH-2078-property-path-negation-and-inversion branch 3 times, most recently from 57d8907 to 14cb209 Compare April 10, 2020 10:45
hmottestad and others added 4 commits April 10, 2020 12:52
Signed-off-by: Håvard Ottestad <hmottestad@gmail.com>
- added test cases to ComplexSPARQLQueryTest
Signed-off-by: Håvard Ottestad <hmottestad@gmail.com>
@hmottestad hmottestad force-pushed the GH-2078-property-path-negation-and-inversion branch from 14cb209 to 44e6367 Compare April 10, 2020 10:57
@hmottestad hmottestad changed the base branch from develop to master April 10, 2020 10:58
@hmottestad hmottestad marked this pull request as ready for review April 10, 2020 10:59
@hmottestad hmottestad merged commit fc4e23a into master Apr 10, 2020
@hmottestad hmottestad deleted the GH-2078-property-path-negation-and-inversion branch April 10, 2020 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wildcard properties
2 participants