More talking#4
Conversation
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Does the specificity of the method name add enough value to justify this duplication? I feel as though (operator-body "IRI" [clauses]) isn't significantly less readable than (iri-body [clauses])
Alternatively, we could create a macro that would enable something like (defoperator iri) to automatically define the appropriate function without the duplication.
|
I've made a few comments in-line. This seems like a quite a nice pragmatic change, given what we have to work with. More generally: would it be possible to add tests cases for the new operators, please? I know that we want to move away from build compliant SPARQL ourselves, but even when happens, we're unlikely to change all of the existing rules, and it'll be important to ensure that these operators continue to behave correctly. The test cases will also also serve as demonstrations of the functionality, which makes it easier to see what they do without having to dig through the rules. |
| (repeat " \n"))) | ||
| (sparql-operator? (first triple-pattern)) (filter-body triple-pattern) | ||
| ;; used as a catch-all; this routes anything unusual to a FILTER. | ||
| ;; (sparql-operator? (first triple-pattern)) (filter-body triple-pattern) |
There was a problem hiding this comment.
This change is going to break backwards compatibility. Before operators were implicitly wrapped in a filter, whereas now the filter body must be explicitly specified.
I really should have seen this earlier ):
There was a problem hiding this comment.
True and not surprising. What is the solution, though? Should we maintain 2 versions, and consider the new one as a sparql 1.1 version? I remember worrying about this issue, but am not sure how to deal with sparql 1.1 syntax when everything is wearing a filter. Maybe tomorrow we can talk more about this.
From: Marc Daya notifications@github.com
Reply-To: UCDenver-ccp/kr reply@reply.github.com
Date: Tuesday, May 31, 2016 at 10:50 AM
To: UCDenver-ccp/kr kr@noreply.github.com
Cc: Elizabeth White elizabeth.white@raritanfarm.com, Author author@noreply.github.com
Subject: Re: [UCDenver-ccp/kr] More talking (#4)
In kr-core/src/main/clojure/edu/ucdenver/ccp/kr/sparql.clj:
@@ -404,14 +466,23 @@
(apply str (interleave (map sparql-query-body triple-pattern)
;;(repeat ". \n")))
(repeat " \n")))
- (sparql-operator? (first triple-pattern)) (filter-body triple-pattern)
+ ;; used as a catch-all; this routes anything unusual to a FILTER.
+ ;; (sparql-operator? (first triple-pattern)) (filter-body triple-pattern)
This change is going to break backwards compatibility. Before operators were implicitly wrapped in a filter, whereas now the filter body must be explicitly specified.
I really should have seen this earlier ):
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
Because it no longer be assumed that all SPARQL functions and operators occur in a `filter` block, `filter` must now be explicitly specified. The change of default select-type default has been reverted, as this breaks the standard query pattern, and is in fact not needed (cf. lhunter-lab#4).
A collection of SPARQL keywords were added both to the body parsing condition, and to the `sparql-operators` map. The latter actually takes precendence; thus the clauses in the condition would never have been hit.
It may be desirable in some instances to pre-validate the arguments that are passed to an operator, rather than only relying on the backend parsing of the generated SPARQL. One example of this arises from how operators are implemented across different backends. For example, the IRI function across the various platforms does not consistent handle language-tagged strings; some (like Sesame) will do the conversion, while others (like Allegrograph) will not. This also provides a hook to insert validatation to help users with the not-quite-SPARQL pattern syntax.
|
I've pushed some clean up commits into this pull request. Please review and let me know if you have any comment, but do not merge until we've had a chance to run the Reactome rules against these changes. |
2 participants
talk pull