Fix set semantics bug #7

maartyman · 2023-10-25T09:39:33Z

No description provided.

coveralls · 2023-10-25T09:40:44Z

Pull Request Test Coverage Report for Build 6657081957

29 of 29 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 4676623935:	0.0%
Covered Lines:	81
Relevant Lines:	81

💛 - Coveralls

maartyman · 2023-10-25T09:45:47Z

High level overview:

Create new PassThrough stream: storeImportStream.
Read the import stream in on-demand mode. For each value check if it has a match.
Await match if it matches, do nothing. If it doesn't match, push into pendingStream and storeImportStream.
Import storeImportStream into store.

maartyman · 2023-10-25T09:46:50Z

A clean-up of the implementation still needs to be done!

test/StreamingStore-test.ts

lib/StreamingStore.ts

…stner

…added test for passing of the error in the import

maartyman · 2023-10-25T14:24:11Z

Everything above has been implemented. Are more tests needed?

rubensworks

Besides the docs comments, everything looks fine, great tests as well 👍

lib/StreamingStore.ts

rubensworks · 2023-10-26T07:04:40Z

@surilindur Before we merge this, is it easy for you to test this in your setup with the aggregate store enabled? If so, can you check if this change solves your problem? If not, you can ignore this comment, and we can check it later for you.

surilindur · 2023-10-26T11:24:17Z

Thank you for looking into a fix!

After changing to the branch here, I stopped getting results from any queries with the aggregate store enabled. After some additional investigation, I found out that using the branch here in base Comunica (master branch) causes this set of unit tests to fail: packages/actor-rdf-resolve-quad-pattern-hypermedia/test/MediatedQuadSource-test.ts

The failure seems to indicate missing quads from a match call (the same test file has two other failing tests):

Summary of all failing tests
 FAIL  packages/actor-rdf-resolve-quad-pattern-hypermedia/test/MediatedQuadSource-test.ts (11.457 s)
  ● MediatedQuadSource › A MediatedQuadSource instance with aggregated store › match › should match three chained sources when queried multiple times

    expected two graphs to be isomorphic.

      Expected:
    [
      {"subject":"s11","predicate":"p11","object":"o11","graph":""},
      {"subject":"s21","predicate":"p21","object":"o21","graph":""},
      {"subject":"s12","predicate":"p12","object":"o12","graph":""},
      {"subject":"s22","predicate":"p22","object":"o22","graph":""},
      {"subject":"s13","predicate":"p13","object":"o13","graph":""},
      {"subject":"s23","predicate":"p23","object":"o23","graph":""}
    ]

      Actual:
    [

    ]

    Missing Quads (that don't contain Blank Nodes):
    [
      {"subject":"s11","predicate":"p11","object":"o11","graph":""},
      {"subject":"s21","predicate":"p21","object":"o21","graph":""},
      {"subject":"s12","predicate":"p12","object":"o12","graph":""},
      {"subject":"s22","predicate":"p22","object":"o22","graph":""},
      {"subject":"s13","predicate":"p13","object":"o13","graph":""},
      {"subject":"s23","predicate":"p23","object":"o23","graph":""}
    ]

    Additional Quads (that don't contain Blank Nodes):
    [

    ]

    Missing Blank Node Patterns:


    Additional Blank Node Patterns:

      254 |         ]);
      255 |         expect(mediatorRdfResolveHypermedia.mediate).toHaveBeenCalledTimes(4);
    > 256 |         expect(await arrayifyStream(source.match(v, v, undefined!, v, context))).toBeRdfIsomorphic([
          |                                                                                  ^
      257 |           quad('s11', 'p11', 'o11'),
      258 |           quad('s21', 'p21', 'o21'),
      259 |           quad('s12', 'p12', 'o12'),

      at Object.<anonymous> (packages/actor-rdf-resolve-quad-pattern-hypermedia/test/MediatedQuadSource-test.ts:256:82)

If I find the time, I can look into what is going wrong, but if someone else has the time before it, that would be amazing!

rubensworks · 2023-10-26T11:27:08Z

Hmm, good catch @surilindur. There might be an edge-case that we're forgetting about.
Do you see any spec/integration tests failing by any chance?

We should probably delay merging this until we know for certain that this doesn't break anything in Comunica.

surilindur · 2023-10-26T11:35:50Z

This one seems to be happening consistently:

 FAIL  engines/query-sparql/test/QuerySparql-test.ts (51.179 s)
  ● System test: QuerySparql › foaf ontology broken link (no browser) › returns results with link recovery on [using full key]

    thrown: "Exceeded timeout of 20000 ms for a test.
    Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

      782 |   // We skip these tests in browsers due to CORS issues
      783 |   describe('foaf ontology broken link (no browser)', () => {
    > 784 |     it('returns results with link recovery on [using full key]', async() => {
          |     ^
      785 |       const result = <QueryBindings> await engine.query(`SELECT * WHERE {
      786 |     <http://xmlns.com/foaf/0.1/> a <http://www.w3.org/2002/07/owl#Ontology>.
      787 |   }`, {

      at engines/query-sparql/test/QuerySparql-test.ts:784:5
      at engines/query-sparql/test/QuerySparql-test.ts:783:3
      at Object.<anonymous> (engines/query-sparql/test/QuerySparql-test.ts:21:1)

But it looks like just a timeout at a first glance?

surilindur · 2023-10-26T11:37:26Z

To clarify, the following cases from the MediatedQuadSource seem to fail:

MediatedQuadSource › A MediatedQuadSource instance with aggregated store › match › should match three chained sources when queried multiple times
MediatedQuadSource › A MediatedQuadSource instance with aggregated store › match › should match three chained sources when queried multiple times in parallel
MediatedQuadSource › A MediatedQuadSource instance with aggregated store › match › should match three chained sources when queried multiple times for multiple queries

And then that random timeout from QuerySparql tests.

rubensworks · 2023-10-26T12:36:41Z

engines/query-sparql/test/QuerySparql-test.ts

@surilindur That one also fails in the CI sometimes. Honestly, I don't know what it is about. I've tried reproducing locally, but to no avail.

So no errors when running the integration and spec tests in query-sparql?

surilindur · 2023-10-26T13:17:46Z

The integration and spec tests in query-sparql seem to pass, yes.

surilindur · 2023-10-26T14:35:15Z

After trying out a lot of things, I realised the aggregate store option is toggled off in base Comunica by default. 😬

Enabling the aggregateStore option with the @default JSDoc value here caused the integration tests to fail.

Example failure in the spoiler here.

✖ SELECT - OPTIONAL FILTER EXISTS
  FILTER EXISTS works within an OPTIONAL block
  Error: Invalid query evaluation

        Query: SELECT * WHERE {
  ?s ?p ?o.
  OPTIONAL {
    FILTER EXISTS {
      ?s2 ?p ?o.
      FILTER (?s != ?s2)
    }
  }
}

        Data: [{"value":"https://comunica.github.io/manifest-ldf-tests/sparql/optional-filter-exists/2triples.ttl","type":"file"}]

        Result Source: https://comunica.github.io/manifest-ldf-tests/sparql/optional-filter-exists/result.srj

        Expected:
 [QueryResultBindings:
    Variables: [
  "?o",
  "?p",
  "?s"
]
    Bindings:  [
  {
    "?s": {
      "termType": "BlankNode",
      "value": "bc_0_b0_blank"
    },
    "?p": {
      "termType": "NamedNode",
      "value": "http://predicate"
    },
    "?o": {
      "termType": "NamedNode",
      "value": "http://object"
    }
  },
  {
    "?s": {
      "termType": "NamedNode",
      "value": "http://subject"
    },
    "?p": {
      "termType": "NamedNode",
      "value": "http://predicate"
    },
    "?o": {
      "termType": "NamedNode",
      "value": "http://object"
    }
  }
]
]

        Got:
 [QueryResultBindings:
    Variables: [
  "?o",
  "?p",
  "?s"
]
    Bindings:  [
  {
    "?s": {
      "termType": "BlankNode",
      "value": "bc_0_b35_blank",
      "skolemized": {
        "termType": "NamedNode",
        "value": "urn:comunica_skolem:source_0:b35_blank"
      }
    },
    "?p": {
      "termType": "NamedNode",
      "value": "http://predicate"
    },
    "?o": {
      "termType": "NamedNode",
      "value": "http://object"
    }
  }
]
]

Hopefully that helps. Sorry about the confusion earlier.

rubensworks · 2023-10-26T14:36:42Z

After enabling the aggregateStore option with the @default JSDoc value here, most of the integration tests started failing.

That is to be expected. The aggregateStore is not compatible with TPF-like sources.

surilindur · 2023-10-26T15:30:35Z

Oh okay, thank you! I had no idea. Sorry. 😦 What if I use query-sparql to query a single turtle document? Should the aggregated store support that?

With the aggregate store enabled, query-sparql fails to produce results for this, but without the aggregate store it works:

node bin/query.js https://comunica.github.io/manifest-ldf-tests/sparql/starwars-valuesbind/starwars.ttl -q 'SELECT ?person ?hair_colour WHERE {
  ?person <http://swapi.dev/documentation#name> "Luke Skywalker" ;
    <http://swapi.dev/documentation#hair_color> ?hair_colour .
}'

The same behaviour seems to happen with the link traversal repository, as well. With just one triple pattern it works, but with two or more it fails.

lib/StreamingStore.ts

rubensworks · 2023-10-27T06:35:04Z

The same behaviour seems to happen with the link traversal repository, as well. With just one triple pattern it works, but with two or more it fails.

@surilindur So this fails only with the new aggregate store from this PR, but not with the old aggregate store? Or does it fail on both?

surilindur · 2023-10-27T07:21:15Z

After checking it again, using the query from the previous comment here, with the aggregate store enabled:

query-sparql fails to produce the expected result with both master and fix-set-semantics-bug branch, which probably means that is supposed to happen and everything is fine?
query-sparql-solid does the same as query-sparql
query-sparql-link-traversal produces the expected result with master, but fails to produce any results with fix-set-semantics-bug

So if the non-link-traversal engines are not supposed to work with the aggregate store for querying a turtle document, then everything is fine. Why the link traversal one works with master branch and not the fix, I do not know.

This is really confusing to me, so apologies for the spam and confusion.

rubensworks · 2023-10-27T07:26:32Z

query-sparql fails to produce the expected result with both master and fix-set-semantics-bug branch, which probably means that is supposed to happen and everything is fine?

The aggregate store was designed mainly for link traversal, and is untested for regular querying. I would expect it to only break for TPF queries, but I may be forgetting about something, and the general breakage is not surprising.
So indeed, all is fine here.

query-sparql-solid does the same as query-sparql

👌

query-sparql-link-traversal produces the expected result with master, but fails to produce any results with fix-set-semantics-bug

This is not ok indeed. And what exactly fails here? So the unit tests pass?

surilindur · 2023-10-27T07:31:31Z

I think I forgot to rebuild the link traversal engine because it seems to work now with the fix branch, as well.

But it does not make sense, because I definitely did build it with the fix branch when some of the queries failed.

maartyman added 3 commits October 20, 2023 20:17

First attempt at fix (not working)

b77d794

partial solution but still not working

cdc3779

first working (but not properly implemented) version

faa3fe3

maartyman linked an issue Oct 25, 2023 that may be closed by this pull request

PendingStream and internal store get out of sync due to set-semantics #6

Closed

rubensworks reviewed Oct 25, 2023

View reviewed changes

test/StreamingStore-test.ts Outdated Show resolved Hide resolved

rubensworks reviewed Oct 25, 2023

View reviewed changes

lib/StreamingStore.ts Outdated Show resolved Hide resolved

rubensworks reviewed Oct 25, 2023

View reviewed changes

lib/StreamingStore.ts Outdated Show resolved Hide resolved

rubensworks reviewed Oct 25, 2023

View reviewed changes

lib/StreamingStore.ts Outdated Show resolved Hide resolved

maartyman added 2 commits October 25, 2023 16:20

propagate errors in import, fixed end logic, only remove handleEnd li…

74761c0

…stner

changed expected triples more explicit, added test for slow imports, …

ff97883

…added test for passing of the error in the import

rubensworks requested changes Oct 26, 2023

View reviewed changes

lib/StreamingStore.ts Show resolved Hide resolved

Add jsdoc comment and inline comments for the importToListeners method

d6b67f9

rubensworks reviewed Oct 27, 2023

View reviewed changes

lib/StreamingStore.ts Show resolved Hide resolved

rubensworks merged commit d11c8ce into master Jan 11, 2024
13 checks passed

rubensworks deleted the fix-set-semantics-bug branch January 11, 2024 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix set semantics bug #7

Fix set semantics bug #7

maartyman commented Oct 25, 2023

coveralls commented Oct 25, 2023 •

edited

Loading

maartyman commented Oct 25, 2023

maartyman commented Oct 25, 2023 •

edited

Loading

maartyman commented Oct 25, 2023 •

edited

Loading

rubensworks left a comment

rubensworks commented Oct 26, 2023

surilindur commented Oct 26, 2023

rubensworks commented Oct 26, 2023

surilindur commented Oct 26, 2023

surilindur commented Oct 26, 2023

rubensworks commented Oct 26, 2023

surilindur commented Oct 26, 2023

surilindur commented Oct 26, 2023 •

edited

Loading

rubensworks commented Oct 26, 2023

surilindur commented Oct 26, 2023

rubensworks commented Oct 27, 2023

surilindur commented Oct 27, 2023 •

edited

Loading

rubensworks commented Oct 27, 2023

surilindur commented Oct 27, 2023

Fix set semantics bug #7

Fix set semantics bug #7

Conversation

maartyman commented Oct 25, 2023

coveralls commented Oct 25, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6657081957

💛 - Coveralls

maartyman commented Oct 25, 2023

maartyman commented Oct 25, 2023 • edited Loading

maartyman commented Oct 25, 2023 • edited Loading

rubensworks left a comment

Choose a reason for hiding this comment

rubensworks commented Oct 26, 2023

surilindur commented Oct 26, 2023

rubensworks commented Oct 26, 2023

surilindur commented Oct 26, 2023

surilindur commented Oct 26, 2023

rubensworks commented Oct 26, 2023

surilindur commented Oct 26, 2023

surilindur commented Oct 26, 2023 • edited Loading

rubensworks commented Oct 26, 2023

surilindur commented Oct 26, 2023

rubensworks commented Oct 27, 2023

surilindur commented Oct 27, 2023 • edited Loading

rubensworks commented Oct 27, 2023

surilindur commented Oct 27, 2023

coveralls commented Oct 25, 2023 •

edited

Loading

maartyman commented Oct 25, 2023 •

edited

Loading

maartyman commented Oct 25, 2023 •

edited

Loading

surilindur commented Oct 26, 2023 •

edited

Loading

surilindur commented Oct 27, 2023 •

edited

Loading