search

LDflex / Query-Solid

Simple access to data in Solid pods through LDflex expressions
https://solid.github.io/query-ldflex/
MIT License
66 stars 15 forks source link

Make blank nodes reusable across expressions #64

Closed RubenVerborgh closed 4 years ago

RubenVerborgh commented 4 years ago

Closes #34.

RubenVerborgh commented 4 years ago

@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 commented 4 years ago

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

rubensworks commented 4 years ago

Updating to Comunica 1.12.1 will fix the problem 🎉

RubenVerborgh commented 4 years ago

@rubensworks Thanks a lot, will check!

RubenVerborgh commented 4 years ago

@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 commented 4 years ago

@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 54128f097f72c41e75f0d2e1bd265485b80bccad should work, yet it does with 1.12.1 (didn't with 1.11.0). What do you think?

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 192

Totals Coverage Status
Change from base Build 186: 0.0%
Covered Lines: 136
Relevant Lines: 136
💛 - Coveralls
RubenVerborgh commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

rubensworks commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.