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.

Sign up for GitHub

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

Streamline typing and implementation of SPARQL processor #2301

Merged
merged 16 commits into from
Apr 7, 2023
Merged

Streamline typing and implementation of SPARQL processor #2301

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
58e34c4
Update processor.py
cthoyt Mar 19, 2023
ba256a0
Update evaluate.py
cthoyt Mar 19, 2023
19f03c3
Update processor.py
cthoyt Mar 19, 2023
bc58b1f
Update processor.py
cthoyt Mar 20, 2023
f686d74
Update processor.py
cthoyt Mar 20, 2023
dba8c0b
Update query.py
cthoyt Mar 20, 2023
633c062
Fix typing in remaining processor code
cthoyt Mar 20, 2023
d5b8d9a
Update processor.py
cthoyt Mar 20, 2023
c444b41
Update processor.py
cthoyt Mar 20, 2023
5dc52cf
Update update.py
cthoyt Mar 20, 2023
ded8847
Merge branch 'main' into patch-2
aucampia Mar 26, 2023
259c10d
Revert optionals
cthoyt Mar 26, 2023
0ad0cff
Update query.py
cthoyt Mar 26, 2023
aeff7a6
Merge branch 'main' into patch-2
aucampia Mar 28, 2023
99432ec
Revert "Revert optionals"
aucampia Mar 28, 2023
ea600b2
restore `rdflib.query.Processor` to its previous state
aucampia Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions rdflib/plugins/sparql/evaluate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ def evalDescribeQuery(ctx: QueryContext, query) -> Dict[str, Union[str, Graph]]:
def evalQuery(
graph: Graph,
query: Query,
initBindings: Mapping[str, Identifier],
initBindings: Optional[Mapping[str, Identifier]] = None,
base: Optional[str] = None,
) -> Mapping[Any, Any]:
"""
Expand All @@ -661,7 +661,7 @@ def evalQuery(
documentation.
"""

initBindings = dict((Variable(k), v) for k, v in initBindings.items())
initBindings = dict((Variable(k), v) for k, v in (initBindings or {}).items())

ctx = QueryContext(graph, initBindings=initBindings)

Expand Down
34 changes: 20 additions & 14 deletions rdflib/plugins/sparql/processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,30 @@


def prepareQuery(
queryString: str, initNs: Mapping[str, Any] = {}, base: Optional[str] = None
queryString: str,
initNs: Optional[Mapping[str, Any]] = None,
base: Optional[str] = None,
) -> Query:
"""
Parse and translate a SPARQL Query
"""
if initNs is None:
initNs = {}
ret = translateQuery(parseQuery(queryString), base, initNs)
ret._original_args = (queryString, initNs, base)
return ret


def prepareUpdate(
updateString: str, initNs: Mapping[str, Any] = {}, base: Optional[str] = None
updateString: str,
initNs: Optional[Mapping[str, Any]] = None,
base: Optional[str] = None,
) -> Update:
"""
Parse and translate a SPARQL Update
"""
if initNs is None:
initNs = {}
ret = translateUpdate(parseUpdate(updateString), base, initNs)
ret._original_args = (updateString, initNs, base)
return ret
Expand All @@ -43,8 +51,8 @@ def prepareUpdate(
def processUpdate(
graph: Graph,
updateString: str,
initBindings: Mapping[str, Identifier] = {},
initNs: Mapping[str, Any] = {},
initBindings: Optional[Mapping[str, Identifier]] = None,
initNs: Optional[Mapping[str, Any]] = None,
base: Optional[str] = None,
) -> None:
"""
Expand Down Expand Up @@ -73,8 +81,8 @@ def __init__(self, graph):
def update(
self,
strOrQuery: Union[str, Update],
initBindings: Mapping[str, Identifier] = {},
initNs: Mapping[str, Any] = {},
initBindings: Optional[Mapping[str, Identifier]] = None,
initNs: Optional[Mapping[str, Any]] = None,
) -> None:
"""
.. caution::
Expand Down Expand Up @@ -108,8 +116,8 @@ def __init__(self, graph):
def query( # type: ignore[override]
self,
strOrQuery: Union[str, Query],
initBindings: Mapping[str, Identifier] = {},
initNs: Mapping[str, Any] = {},
initBindings: Optional[Mapping[str, Identifier]] = None,
initNs: Optional[Mapping[str, Any]] = None,
base: Optional[str] = None,
DEBUG: bool = False,
) -> Mapping[str, Any]:
Expand All @@ -132,9 +140,7 @@ def query( # type: ignore[override]
documentation.
"""

if not isinstance(strOrQuery, Query):
parsetree = parseQuery(strOrQuery)
query = translateQuery(parsetree, base, initNs)
else:
query = strOrQuery
return evalQuery(self.graph, query, initBindings, base)
if isinstance(strOrQuery, str):
strOrQuery = translateQuery(parseQuery(strOrQuery), base, initNs)

return evalQuery(self.graph, strOrQuery, initBindings, base)
6 changes: 4 additions & 2 deletions rdflib/plugins/sparql/update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,9 @@ def evalCopy(ctx: QueryContext, u: CompValue) -> None:


def evalUpdate(
graph: Graph, update: Update, initBindings: Mapping[str, Identifier] = {}
graph: Graph,
update: Update,
initBindings: Optional[Mapping[str, Identifier]] = None,
) -> None:
"""

Expand Down Expand Up @@ -315,7 +317,7 @@ def evalUpdate(
"""

for u in update.algebra:
initBindings = dict((Variable(k), v) for k, v in initBindings.items())
initBindings = dict((Variable(k), v) for k, v in (initBindings or {}).items())

ctx = QueryContext(graph, initBindings=initBindings)
ctx.prologue = u.prologue
Expand Down
4 changes: 2 additions & 2 deletions rdflib/query.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ def __init__(self, graph: "Graph"):
def query( # type: ignore[empty-body]
self,
strOrQuery: Union[str, "Query"], # noqa: N803
initBindings: Mapping["str", "Identifier"] = {}, # noqa: N803
initNs: Mapping[str, Any] = {}, # noqa: N803
initBindings: Optional[Mapping["str", "Identifier"]] = None, # noqa: N803
initNs: Optional[Mapping[str, Any]] = None, # noqa: N803
Copy link
Member

Choose a reason for hiding this comment

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

While the current function signature is problematic due to its use of mutable data structures in argument defaults, One problem with changing this is, plugins that implement this as it stands could break if they now receive valid values according to the new function signature, as they don't expect None as a value.

We need to figure out a way to upgrade our plugin interfaces, I think the right approach is to create ProcessorV2, but we need to also consider what other changes we want to introduce before that.

I think it is best to change this back to what it was, though the rest of the PR seems good and quite helpful.

Suggested change
initBindings: Optional[Mapping["str", "Identifier"]] = None, # noqa: N803
initNs: Optional[Mapping[str, Any]] = None, # noqa: N803
initBindings: Mapping["str", "Identifier"] = {}, # noqa: N803
initNs: Mapping[str, Any] = {}, # noqa: N803

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see how this could be an issue. I will switch the defaults to retain the empty dictionary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 259c10d and 0ad0cff

Copy link
Member

Choose a reason for hiding this comment

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

I don't think 259c10d is really needed, I think it is good to keep it as you had it, because those are not part of our plugin interface.

The only part of your change that relates to the interface is the change to rdflib.query.Processor, and here the most critical thing is that we don't want other parts of RDFLib to call plugins in a way or with arguments that they don't support, so if RDFLib code calls rdflib.query.Processor.query with None value for initBindings or initNs then that should result in a type error from mypy.

I will revert 259c10d I think, and then remove the optionals in rdflib.query.Processor, I will ping you when it is done so you can just confirm that you are happy with it.

Copy link
Member

Choose a reason for hiding this comment

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

Done now, please have a look and let me know if things look okay, but essentially I just reverted 259c10d and then changed rdflib/query.py back to what is in the main branch.

DEBUG: bool = False,
) -> Mapping[str, Any]:
pass
Expand Down