search

comunica / sparqlee

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

Breaking changes #122

Closed rubensworks closed 2 years ago

rubensworks commented 3 years ago

@jitsedesmet It looks like something went wrong while checking whether or not changes in sparqlee caused breakages in Comunica. Everything from the master HEAD until this commit causes unit, spec and integration test failures.

I have no bandwidth to look into fixing all of the problems. So my current idea is to revert master back to the state before the OverloadTree was added, as everything still worked there, and there were no performance regressions yet. Unless you see any other solutions @jitsedesmet?

jitsedesmet commented 3 years ago

Oh, I'm really sorry to hear that. I'm out for the week so I think it would be best to revert the changes until then. I will take a closer look at the issues when I'm back. I must have done something wrong, sorry!

jitsedesmet commented 3 years ago

A does the mentioned commit work? It seems like it's the commit where we introduced the new cache. Maybe I needed to add this module dependency somewhere and didn't?

rubensworks commented 3 years ago

No, that one doesn't work. I'll just revert (as I have to prepare a new release ASAP)

jitsedesmet commented 3 years ago

With even more pain in my hearth I suggest rolling back further than this then, to the commit before we touched the type system since the new type system without a cache is way to slow.

rubensworks commented 3 years ago

Up until what commit would you suggest then?

jitsedesmet commented 3 years ago

Since I'm one my phone it's pretty hard to get the reference but here https://github.com/comunica/sparqlee/pull/112#discussion_r695577156 I mentioned a few commits, the commit with lowest execution time is the one we want. This is where we introduced the overloadTree but didn't touch the type system.

jitsedesmet commented 3 years ago

I think it was this commit https://github.com/jitsedesmet/sparqlee/commit/c4959d634995a1f5724d148b22880ed60b92da23. Again, I'm really sorry for all this.

rubensworks commented 3 years ago

Ah yeah, that's the one I was referring to in the issue description :-) So I'll just revert to right before that one.

rubensworks commented 3 years ago

HEAD state at the time of writing is now in https://github.com/comunica/sparqlee/commits/reverted/jitse, and master has been reverted.