comunica / sparqlee

⚙️ SPARQL expression evaluator library - Moved to @comunica/expression-evaluator
https://www.npmjs.com/package/sparqlee
14 stars 7 forks source link

Feature/async aggregate evaluator (closes #86) #88

Closed jitsedesmet closed 3 years ago

jitsedesmet commented 3 years ago

Iimplementation of an async version of the existing AggragateEvaluator as explained in #86 .

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1082191114


Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/evaluators/AggregateEvaluator.ts 47 57 82.46%
<!-- Total: 47 57 82.46% -->
Totals Coverage Status
Change from base Build 1077926499: -0.1%
Covered Lines: 1163
Relevant Lines: 1388

💛 - Coveralls
rubensworks commented 3 years ago

Looks good at first glance! Could you check if this works as intended in Comunica (perhaps with a good test case)? I can have a more detailed look at this afterwards.

jitsedesmet commented 3 years ago

You're right, I removed the init because that didn't make sense for me either. You're right however that it would be cleaner if put just calls init and does nothing else. I thought the diffrence for GroupConcat was made clear in the README?

wschella commented 3 years ago

You're right, I removed the init because that didn't make sense for me either. You're right however that it would be cleaner if put just calls init and does nothing else. I thought the diffrence for GroupConcat was made clear in the README?

I'm sorry, I read over the README changes somehow.

jitsedesmet commented 3 years ago

I just pushed a commit in which I also added a function that allows users to evaluate a term and put the term in the aggregator themself with putTerm. I did this because evaluation, especially with the recently added extension functions in async mode, could take some time. This allows a user to control this evaluation when he/she would feel the need for it.

jitsedesmet commented 3 years ago

@wschella I just bumped into some weird behavior which made me wonder, why doesn't safeThrow set a boolean, 'errorOccured' which the evaluator checks?

wschella commented 3 years ago

@wschella I just bumped into some weird behavior which made me wonder, why doesn't safeThrow set a boolean, 'errorOccured' which the evaluator checks?

I don't quite understand the question. The reason it's implemented like this is likely some spec thing I should have documented. I think in spec defined behavior when an error occurs they aren't supposed to be thrown and the result is simply undefined. That's what implemented + a setting to throw errors if you explicitly do want them.

Whether that's wishful is one thing, but note you can have many many errors, and silent errors for some parts of the stream are true footguns, so this isn't too bad I think.

jitsedesmet commented 3 years ago

Yes, I now realise my question was actually not really valid for the code you implemented. It was a way async silent errors are to be handled here it is necessary to have a boolean. Don't worry about it ;) (well I hope)