Issue 166 in SPARQLWrapper solved #1900

PritishWadhwa · 2022-05-09T08:52:50Z

The order of ResultRows for a SELECT * query was randomized and was changing. This is unexpected because other SPARQL engines seem to generally apply the order of variables as they occur in the WHERE block.

Sample code:

query="SELECT * { ?a ?b ?c } LIMIT 10"
qres=g.query(query)
print(qres.vars)

Expected output:
[rdflib.term.Variable('a'), rdflib.term.Variable('b'), rdflib.term.Variable('c')]

Real output:
[rdflib.term.Variable('c'), rdflib.term.Variable('a'), rdflib.term.Variable('b')] (first run)
[rdflib.term.Variable('a'), rdflib.term.Variable('b'), rdflib.term.Variable('c')] (second run)
[rdflib.term.Variable('b'), rdflib.term.Variable('a'), rdflib.term.Variable('c')] (third run)
[rdflib.term.Variable('a'), rdflib.term.Variable('c'), rdflib.term.Variable('b')] (fourth run)
[you get the idea]

(Tested on the HDT edition of DBpedia 2016, created with g = rdflib.Graph(store=rdflib_hdt.HDTStore(rdf_file)), but that shouldn't matter.)

This was corrected without interfering with the time complexity and the output is now as it should be (as shown in the expected output)

aucampia · 2022-05-09T12:40:20Z

All changes I see is the addition of a commented function, maybe something is missing? If you are still working on a PR best to mark it as draft.

coveralls · 2022-05-09T12:49:30Z

Coverage increased (+0.005%) to 88.243% when pulling d2add04 on PritishWadhwa:Issue166 into eba1373 on RDFLib:master.

PritishWadhwa · 2022-05-09T12:52:26Z

All changes I see is the addition of a commented function, maybe something is missing? If you are still working on a PR best to mark it as draft.

Yes I was finalizing something, it is done now

aucampia · 2022-05-09T13:18:02Z

All changes I see is the addition of a commented function, maybe something is missing? If you are still working on a PR best to mark it as draft.

Yes I was finalizing something, it is done now

Many tests fail now:

https://drone.rdflib.ashs.dev/RDFLib/rdflib/1685/1/2

= 56 failed, 5417 passed, 2 skipped, 117 xfailed, 1 xpassed, 50 warnings in 264.39s (0:04:24) =

Also this PR will need a test that reproduces the broken behavior for it to be merged.

…to Issue166

PritishWadhwa · 2022-05-10T06:14:48Z

I have made the changes, now the tests which were earlier failing should not fail. I have also added the test file for reproducing the broken behavior.

ghost · 2022-05-10T09:47:17Z

Looking good, all that's left to do in order to satisfy the automated contrib tests is to black the test file.

PritishWadhwa · 2022-05-10T13:30:09Z

Looking good, all that's left to do in order to satisfy the automated contrib tests is to black the test file.

Hi. Can you please explain what you mean by "blacking the test file" and how to do it? I am fairly new to open source contribution and haven't seen this term before.

aucampia · 2022-05-10T14:23:11Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

aucampia · 2022-05-10T14:23:50Z

pre-commit.ci autofix

@PritishWadhwa this is what @gjhiggins meant

I will review this when I can, before Sunday if possible.

aucampia · 2022-05-10T14:25:03Z

test/test_issues/test_issue166.py

+from rdflib import Graph
+
+
+def test_issue_166() -> None:


Just briefly, I would much rather prefer this goes into

rdflib/test/test_sparql/test_sparql.py

Lines 694 to 727 in eba1373

pytest.param(

"""

SELECT * WHERE {

BIND(

NOT EXISTS {

rdfs:Class rdfs:label "Class"

}

AS ?bound

)

}

""",

[{Variable('bound'): Literal(False)}],

id="select-bind-notexists-const-true",

),

],

)

def test_queries(

query_string: str,

expected_bindings: Sequence[Mapping["Variable", "Identifier"]],

rdfs_graph: Graph,

) -> None:

"""

Results of queries against the rdfs.ttl return the expected values.

"""

query_tree = parseQuery(query_string)

logging.debug("query_tree = %s", prettify_parsetree(query_tree))

logging.debug("query_tree = %s", query_tree)

query = translateQuery(query_tree)

logging.debug("query = %s", query)

query._original_args = (query_string, {}, None)

result = rdfs_graph.query(query)

logging.debug("result = %s", result)

assert expected_bindings == result.bindings

aucampia · 2022-05-11T22:08:21Z

Appologies but this will only be reviewed in W20 as #1891 is taking most of my time and I have other priorities also.

PritishWadhwa · 2022-05-12T01:00:50Z

Appologies but this will only be reviewed in W20 as #1891 is taking most of my time and I have other priorities also.

Sure, No worries!

aucampia · 2022-05-12T15:53:44Z

Just a note, to link this PR to an issue please see:

Linking a pull request to an issue

The way you are doing it makes more involved to find the issue you are referring to.

aucampia · 2022-05-13T20:32:32Z

I'm going to first merge typing from #1850 before merging these fixes, this will be in smallish PRs like:

feat: add typing for rdflib/plugins/sparql #1926

Once these are merged you should rebase this.

In general though the tests should still be integrated.

Furthermore, I think it may be better to use dict or OrderedDict instead of list and a set: https://stackoverflow.com/a/53657523/1598080

PritishWadhwa · 2022-05-14T05:28:53Z

I'm going to first merge typing from #1850 before merging these fixes, this will be in smallish PRs like:

Add typing for rdflib/plugins/sparql #1926

Once these are merged you should rebase this.

In general though the tests should still be integrated.

Furthermore, I think it may be better to use dict or OrderedDict instead of list and a set: https://stackoverflow.com/a/53657523/1598080

Sure

aucampia

See previous comments.

aucampia · 2022-05-15T14:11:40Z

Please rebase this branch onto master or merge master into this branch.

aucampia · 2022-07-22T01:18:15Z

If there is nothing further on this PR I will close it.

aucampia · 2022-07-29T02:12:21Z

Closing this PR due to inactivity, feel free to open it again if you continue working on it again.

aucampia · 2022-07-29T02:13:36Z

Closing this PR due to inactivity, feel free to open it again if you continue working on it again.

PritishWadhwa and others added 3 commits May 9, 2022 14:17

Issue 166 in SPARQLWrapper solved

6bbfcd5

Update algebra.py

15d9736

Update algebra.py

f92a043

Update algebra.py

0a1f04a

PritishWadhwa added 2 commits May 10, 2022 11:25

Issue 166 in SPARQLWrapper solved

288fc47

Merge branch 'Issue166' of https://github.com/PritishWadhwa/rdflib in…

0f438d2

…to Issue166

ananya2711 approved these changes May 10, 2022

View reviewed changes

[pre-commit.ci] auto fixes from pre-commit.com hooks

d2add04

for more information, see https://pre-commit.ci

aucampia reviewed May 10, 2022

View reviewed changes

PritishWadhwa mentioned this pull request May 12, 2022

Order of ResultRows for SELECT * RDFLib/sparqlwrapper#166

Open

aucampia requested changes May 14, 2022

View reviewed changes

aucampia added the needs more work The PR needs more work before being merged or further reviewed. label May 14, 2022

aucampia closed this Jul 29, 2022

aucampia reopened this Jul 29, 2022

aucampia closed this Jul 29, 2022

	pytest.param(
	"""
	SELECT * WHERE {
	BIND(
	NOT EXISTS {
	rdfs:Class rdfs:label "Class"
	}
	AS ?bound
	)
	}
	""",
	[{Variable('bound'): Literal(False)}],
	id="select-bind-notexists-const-true",
	),
	],
	)
	def test_queries(
	query_string: str,
	expected_bindings: Sequence[Mapping["Variable", "Identifier"]],
	rdfs_graph: Graph,
	) -> None:
	"""
	Results of queries against the rdfs.ttl return the expected values.
	"""
	query_tree = parseQuery(query_string)

	logging.debug("query_tree = %s", prettify_parsetree(query_tree))
	logging.debug("query_tree = %s", query_tree)
	query = translateQuery(query_tree)
	logging.debug("query = %s", query)
	query._original_args = (query_string, {}, None)
	result = rdfs_graph.query(query)
	logging.debug("result = %s", result)
	assert expected_bindings == result.bindings

Issue 166 in SPARQLWrapper solved #1900

Issue 166 in SPARQLWrapper solved #1900

Uh oh!

Conversation

PritishWadhwa commented May 9, 2022

Uh oh!

aucampia commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PritishWadhwa commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aucampia commented May 9, 2022

Uh oh!

PritishWadhwa commented May 10, 2022

Uh oh!

ghost commented May 10, 2022

Uh oh!

PritishWadhwa commented May 10, 2022

Uh oh!

aucampia commented May 10, 2022

Uh oh!

aucampia commented May 10, 2022

Uh oh!

aucampia May 10, 2022

Choose a reason for hiding this comment

Uh oh!

aucampia commented May 11, 2022

Uh oh!

PritishWadhwa commented May 12, 2022

Uh oh!

aucampia commented May 12, 2022

Uh oh!

aucampia commented May 13, 2022

Uh oh!

PritishWadhwa commented May 14, 2022

Uh oh!

aucampia left a comment

Choose a reason for hiding this comment

Uh oh!

aucampia commented May 15, 2022

Uh oh!

aucampia commented Jul 22, 2022

Uh oh!

aucampia commented Jul 29, 2022

Uh oh!

aucampia commented Jul 29, 2022

Uh oh!

Uh oh!

aucampia commented May 9, 2022 •

edited

Loading

coveralls commented May 9, 2022 •

edited

Loading

PritishWadhwa commented May 9, 2022 •

edited

Loading