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.

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

Feature/query iri map #452

Merged
merged 11 commits into from
Apr 17, 2023
Merged

Feature/query iri map #452

merged 11 commits into from
Apr 17, 2023

Conversation

zonotope
Copy link
Contributor

This patch allows users to reference subjects with an iri map in the object position containing one entry whose key is "@id" and value is the referenced iri.

After parsing the iri map, the query executor first looks up the subject id corresponding to the supplied iri and then processes the rest of the query normally.

@zonotope zonotope added this to the 3.0.0-alpha2 milestone Apr 13, 2023
@zonotope zonotope requested a review from a team April 13, 2023 03:34
@zonotope zonotope self-assigned this Apr 13, 2023
@@ -41,6 +41,10 @@
[x]
(boolean (#{'desc "desc" :desc} x)))

(defn iri-key?
[x]
(= "@id" x))
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a constant for the @id string (const/iri-id) that we may want to use here.

@cap10morgan
Copy link
Contributor

In other places where this construct is used (transaction data mainly) it is often with "id" "@id" in the context and so these can be written as either {"id" ...} or {"@id" ...}. I bet one of the first things that would come up from users of this is "Why the inconsistency?" It's a bit tricky, though. Since transaction data is JSON-LD those get expanded for you. But queries aren't JSON-LD, so you don't get that for free. But I do wonder how hard it would be to support context-aliases for "@id" in here? Perhaps not a this-PR thing either way, just wanted to flag it.

@bplatz
Copy link
Contributor

bplatz commented Apr 13, 2023

I think this is good to go, but not fully accurate. Based on the context being used, we don't know what key will translate to @id. If this is run after json-ld expansion, it will always be @id. If run before expansion, it could literally be anything (we often alias id to @id in our example contexts).

One optimization we did in the JSON-LD library is we include :type-key so we don't have to, in some scenarios, fully expand to perform some logic. I think we should also consider including :id-key in the parser. Then this logic could be as an example:

(defn iri-key?
  [x parsed-context]
  (or (= (:id-key parsed-context) x)
        (= "@id" x))

The way it is I think we should merge though, so we can make this work tomorrow, but make another ticket to enhance the json-ld library, and then modify this once that is done.

@zonotope
Copy link
Contributor Author

When writing this code, I had assumed that this "@id" key would be expanded by the time it got to this part of the code, but there's nothing here right now that does the expanding. I think this code is fine as is (or with minor changes anyone would like to suggest) because this is the only thing that can show up as a map like this in the object position.

I do however like the idea of using this map format for more than just specifying an iri eventually, so we'd have to figure out proper id key expansion before we can do that.

@zonotope
Copy link
Contributor Author

I've made changes to expand the key of the iri map as well so the equality check against "@id" should always work.

Copy link
Contributor

@cap10morgan cap10morgan left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@zonotope zonotope merged commit f04ec1c into main Apr 17, 2023
@zonotope zonotope deleted the feature/query-iri-map branch April 17, 2023 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants