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

Refactor the Triple variant of Subject and Term #100

Open
pchampin opened this issue Sep 2, 2022 · 3 comments
Open

Refactor the Triple variant of Subject and Term #100

pchampin opened this issue Sep 2, 2022 · 3 comments

Comments

@pchampin
Copy link
Collaborator

pchampin commented Sep 2, 2022

Subject and Term both have a variant that goes:

    Triple(&'a Triple<'a>)

I am extending Sophia to use [TriplesFormatter], and I find the above very inconvenient. Everytime I want to create a Subject or Term "view" of my internal terms, if that term is a quoted triple, I need to allocate a Rio Triple somewhere that I can then reference. And I have to do this recursively, which is a pain. (I was unable to achieve that without some unsafe code...)

Did you run into a similar problem with Oxigraph? Did you find an elegant way to solve it?

I would suggest to change the variant in Subject and Term

    Triple(Box<Triple<'a>>)

I think that the code of the parser would be minimally impacted by that change, and it would make it much easier to use the formatters.

If you agree with the general idea, I can propose a PR.

@Tpt
Copy link
Collaborator

Tpt commented Sep 2, 2022

Did you run into a similar problem with Oxigraph? Did you find an elegant way to solve it?

Yes, I ran into a similar problem and I have not found a way to elegantly solve it. I use Triple(Box<Triple<'a>>) just like you suggested inside of oxrdf.

If you agree with the general idea, I can propose a PR.

Yes, feel free to do so.

@pchampin
Copy link
Collaborator Author

Just gave it a quick try, but unfortunately, this change is much more disruptive than I anticipated...

  • if we go that way, Subject and Term can not implement Copy anymore,
  • so Triple and Quad can't implement Copy either,
  • but the whole API assumes that Triple and Quad are copy, because the on_triple method passed to TriplesParser expects a Triple passed by value, not by reference (and similarly for QuadParser).

This would force implementation of TriplesParser to clone the triple before passing it to on_triple, possibly causing recursive clones of the boxed quoted triples, which would go against all the optimizations done by Rio for atomic terms...

@Tpt
Copy link
Collaborator

Tpt commented Oct 18, 2022

Thank you for trying!

I am moving forward well with my new N3/Turtle/N-Triples parser. It seems to be at least twice as fast as Rio. So, if you are fine to wait a bit I hope to release it in the next few month.

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

No branches or pull requests

2 participants