Closed dwhitney closed 6 years ago
Hi
A bit of context first. All the architecture follows the pipeline paradigm, popularized by database systems. I use it here because it avoids a lot of data materialization and allows to read solutions in a stream (either in a pull or push fashion). Currently, I use the asynciterator
package because it provides so many useful primitives to work with the paradigm.
However, I agree with you that I should not require users to use specifically asynciterator
to implements a subclass of Graph
. I have an idea to solve this: asynciterator
can also be used with Node.js Streams. Do you think that making methods Graph.find
and Graph.evalBGP
returns an interface compatible with Readable
Stream could solve this?
an interface compatible with Readable
Stream would be great!
I've pushed changes to a new readable
branch that implement this new API. Feel free to give it a look! I will wait a couple of days before merging it, to see if any changes in the API are required.
Hey I tried it out and it works pretty well. I noticed that you still require AsyncIterator on evalBGP
? Is that going to change?
Thank you, nice to hear that it works :) About evalBGP
, I don't see what could prevent me to change the API to return a Readable
as well. I will investigate it.
While I'm on it, di you have any other API suggestion in mind ?
Okay, I changed the Graph.evalBGP
API in d72d46d491db0c4b739146b9d52c360a98e9851a.
I underestimated the strength of the framework's architecture ^^
My concerns were more practical than architectural - I'm using this in the
browser, so bundle size is a concern for me. It seemed to me that this
could be done with streams instead of iterators, which would free up that
dependency, and looking through the other dependencies it seems the only
one you really "need" is sparqljs
. That said, I've only skimmed the
codebase so I could be off base. Either way I have been enjoying this
library!
D
On Tue, Oct 2, 2018 at 2:50 AM Thomas Minier notifications@github.com wrote:
Okay, I changed the Graph.evalBGP API in d72d46d https://github.com/Callidon/sparql-engine/commit/d72d46d491db0c4b739146b9d52c360a98e9851a . I underestimated the strength of the framework's architecture ^^
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Callidon/sparql-engine/issues/3#issuecomment-426167502, or mute the thread https://github.com/notifications/unsubscribe-auth/AACCoXAO8nptHxCsLEMB3wowuH3wio8wks5ugwzHgaJpZM4W1TjS .
Well, not really, since Node.js streams are not native in browsers and require a polyfill. The main reason for using asynciterator
is because it is much more easy to work with than Node.js streams.
I hear you on the polyfill, but I don't think they are easier. I had a hell of a time making an iterator from a Promise (no docs on how to make your own iterator), and I'm having problems that I suspect are related to my implementation of the iterator, but I'm not quite sure... hopefully I figure it out soon!
Have you had a look at funfix? https://github.com/funfix/funfix I've had a lot of good experiences with it.
In any case - I think I should say that if you want to continue using
asnynciterator
as a dependency, you can discard my idea of abstracting
the return type from find
and evalBGP
since it was mostly about
removing the dependency.
D
On Tue, Oct 2, 2018 at 9:48 AM Thomas Minier notifications@github.com wrote:
Well, not really, since Node.js streams are not native in browsers and require a polyfill. The main reason for using asynciterator is because it is much more easy to work with than Node.js streams.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Callidon/sparql-engine/issues/3#issuecomment-426280052, or mute the thread https://github.com/notifications/unsubscribe-auth/AACCoS4Q4tYMekhuCZ7jodbpDFSXGbTGks5ug26hgaJpZM4W1TjS .
Okay, thank you for the feedback. I will keep the changes to Graph
API, because it is still a good idea from an architectural point of view. We will delay the discussion on the need for the asynciterator
package for later ;-)
Hmm... I'm seeing some pretty significant slowdown in master now. A query that previously took roughly 400ms is now taking 40 seconds. Odd thing is it seems to only be happening in the browser and not in node. Here's a screenshot of some profiling I've been doing. I'll post more as I get more results.
Ouch... I will just forward your comment to a new issue (#6) and we will continue the discussion here.
Hey I've been using this the last few weeks and I really love it! I'm wondering how you'd feel about making the return type from "find" on the graph function more abstract. I'd rather not have to add the dependency of asynciterator to my project and if there were just some class or interface to make sure I returned from "find" then I wouldn't have to. Thoughts?