Skip to content

Conversation

@RubenVerborgh
Copy link
Member

Closes #34.

@RubenVerborgh RubenVerborgh added the feature A new feature to be added label Apr 25, 2020
@RubenVerborgh RubenVerborgh self-assigned this Apr 25, 2020
@RubenVerborgh
Copy link
Member Author

@rubensworks Would .skolemize also work when LDflex queries a single source? Because I see you've introduced it in a federated context (and I can imagine there to be a shortcut when there is only one source).

Concretely:

Then you will see a blank node without .skolemized.

@rubensworks
Copy link
Contributor

Hmm, this should work even for single sources. I'll look into this asap.

rubensworks added a commit to comunica/comunica that referenced this pull request Apr 27, 2020
rubensworks added a commit to comunica/comunica that referenced this pull request Apr 27, 2020
@rubensworks
Copy link
Contributor

Updating to Comunica 1.12.1 will fix the problem 🎉

@RubenVerborgh

This comment has been minimized.

@RubenVerborgh RubenVerborgh force-pushed the feature/reusable-blank-nodes branch from 52dc8db to 478fc4c Compare May 15, 2020 20:40
@RubenVerborgh
Copy link
Member Author

RubenVerborgh commented May 15, 2020

@rubensworks Lol, I've upgraded to 1.12.1 and it just works, without me writing any code.
I'd at least thought I would need to expose skolemized, so now I have to spend time figuring out why it works 😂 But thanks in any case!

@RubenVerborgh RubenVerborgh force-pushed the feature/reusable-blank-nodes branch from 63f6968 to 54128f0 Compare May 15, 2020 21:17
@RubenVerborgh
Copy link
Member Author

RubenVerborgh commented May 15, 2020

@rubensworks I first I thought it was this one:

https://github.com/comunica/comunica/blob/47c80ef2f5c118f3926c3be860865438dd8ba445/packages/actor-rdf-resolve-quad-pattern-federated/lib/FederatedQuadSource.ts#L114-L116

Given that the passed term is an LDflex path which always has skolemized defined (it's a promise to a path that will eventually reject). However, then I realized the in operator is not implemented yet in LDFlex, so that test always yields false (thankfully).

In other words, I don't think 54128f0 should work, yet it does with 1.12.1 (didn't with 1.11.0). What do you think?

@coveralls
Copy link

coveralls commented May 15, 2020

Pull Request Test Coverage Report for Build 192

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 186: 0.0%
Covered Lines: 136
Relevant Lines: 136

💛 - Coveralls

@RubenVerborgh RubenVerborgh marked this pull request as ready for review May 15, 2020 21:20
@RubenVerborgh RubenVerborgh requested a review from rubensworks May 15, 2020 21:20
@RubenVerborgh
Copy link
Member Author

RubenVerborgh commented May 15, 2020

@rubensworks Depending on how the resulting pattern from algebraFactory.createPattern is used, https://github.com/comunica/comunica/blob/47c80ef2f5c118f3926c3be860865438dd8ba445/packages/actor-rdf-resolve-quad-pattern-federated/lib/FederatedQuadSource.ts#L132 might be an error.

After all, a blank node in a pattern should represent a (non-returned) variable, whereas it seems to represent a specific constant here.

…or is this actually the intended behavior, and will blank nodes with the same blank node label always match? In that sense I'm confused by https://github.com/comunica/comunica/blob/47c80ef2f5c118f3926c3be860865438dd8ba445/packages/actor-rdf-resolve-quad-pattern-federated/lib/FederatedQuadSource.ts#L222 . But then skolemized seems to be unnecessary (it's literally not passed by LDflex).

Also, is it safe that you are just minting variables named vs/vp/vo/vg in https://github.com/comunica/comunica/blob/47c80ef2f5c118f3926c3be860865438dd8ba445/packages/actor-rdf-resolve-quad-pattern-federated/lib/FederatedQuadSource.ts#L223-L226 ? What if any of the other components already have that name?

@rubensworks
Copy link
Contributor

Lol, I've upgraded to 1.12.1 and it just works, without me writing any code.

Hmm, weird. I don't think it should work like that, will look into it.

will blank nodes with the same blank node label always match?

Any blank nodes that occur at this stage originate from sources. Any blank nodes from the query will be translated to variables before query exec.

The idea is that blank nodes will be matched by label, but not if they come from different sources, then they will never match.

Also, is it safe that you are just minting variables named vs/vp/vo/vg?

This should be safe, yes, since we're working in quad pattern context here. These variable names are just used to mark wildcards in sources, but are then used to construct quads, so the variable names lever leave this context.

@RubenVerborgh
Copy link
Member Author

The idea is that blank nodes will be matched by label, but not if they come from different sources, then they will never match.

I think that should be: blank nodes are matched by canonical URI if they have one. If not, blanks in a query act like variables.

@rubensworks
Copy link
Contributor

I think that should be: blank nodes are matched by canonical URI if they have one. If not, blanks in a query act like variables.

That should be what is happening now, since query-bnodes will become variables before query exec starts.

@RubenVerborgh
Copy link
Member Author

Evidence seems to point at a comparison with blank node labels being done.

@rubensworks
Copy link
Contributor

I just had a look, and Comunica receives queries like these:

SELECT ?name WHERE {
  <urn:comunica_skolem:source_0:n3-0> <http://xmlns.com/foaf/0.1/name> ?name.
}

Which means that the blank nodes are already skolemized somehow.

I'll look into it a bit further now.

@rubensworks
Copy link
Contributor

Hmm, it looks like LDflex already contains the skolemization logic here: https://github.com/LDflex/LDflex/blob/master/src/SparqlHandler.js#L209-L217

If I disable this, the test fails, as expected.

So I guess there is no issue, and everything is working as intended?
Or am I misunderstanding the problem?

@RubenVerborgh
Copy link
Member Author

My bad. I had forgotten about this piece of code, which apparently uses the same skolemized convention already from before it was in Comunica. It all makes sense now.

So it's actually working as intended. How exciting!

Will finish this up and release soon.

@RubenVerborgh RubenVerborgh deleted the feature/reusable-blank-nodes branch June 1, 2020 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature A new feature to be added

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow blank nodes to be reused across expressions

4 participants