Feature/bgp left deep reordering #102

joachimvh · 2018-03-07T16:34:24Z

This implements the original TPF query algorithm.

(Performance problem I had was because I executed the promises consecutively).

coveralls · 2018-03-07T16:45:06Z

Pull Request Test Coverage Report for Build 49

96 of 96 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 43:	0.0%
Covered Lines:	2060
Relevant Lines:	2060

💛 - Coveralls

rubensworks · 2018-03-07T23:37:40Z

...tor-query-operation-bgp-left-deep-reordering/lib/ActorQueryOperationBgpLeftDeepReordering.ts

+    // Continue clustering as long as different clusters have common variables
+    do {
+      // Find a variable that occurs in more than one subpattern
+      const allVariables = Array.prototype.concat(...clusters.map((cluster) => cluster.variables));


That's a nice shorthand, I'll have to remember that :-)

(Totally stole it from stackoverflow somewhere).

rubensworks

Only some minor comments.

I'm just wondering what the performance of this is, and how it compares to the current BGP actor. Is it always faster/slower? Or only in some cases? If so, would we be able to create a mediator to choose the best actor in each case?

rubensworks · 2018-03-07T23:39:25Z

...tor-query-operation-bgp-left-deep-reordering/lib/ActorQueryOperationBgpLeftDeepReordering.ts

+
+  public getBoundOperationIterator(op: Algebra.Operation, bindings: Bindings, context?: {[id: string]: any})
+  : PromiseProxyIterator<Bindings> {
+    const getStream = async () => {


Why is this needed? You should be able to put this call directly inside the PromiseProxyIt callback as far as I can see?

This felt a bit more readable to me than putting everything in a single block. But yea, no other reason than that.

I would combine it then, as this will save us an unnecessary cb+promise.

rubensworks · 2018-03-07T23:40:11Z

...tor-query-operation-bgp-left-deep-reordering/lib/ActorQueryOperationBgpLeftDeepReordering.ts

+
+    const best = {
+      count: Infinity,
+      index: -1 };


I would place the closing bracket on the next line, feels a bit cleaner IMHO :-)

rubensworks · 2018-03-07T23:41:28Z

...tor-query-operation-bgp-left-deep-reordering/lib/ActorQueryOperationBgpLeftDeepReordering.ts

+  public async runOperation(pattern: Algebra.Bgp, context?: {[id: string]: any})
+  : Promise<IActorQueryOperationOutputBindings> {
+    const clusters = ActorQueryOperationBgpLeftDeepReordering.findConnectedPatterns(pattern.patterns);
+    // find cluster with least unbound variables (with the least patterns in case of a tie)


Your comments after here don't start with a capital letter anymore, I would add them for consistency.

rubensworks · 2018-03-07T23:43:09Z

...tor-query-operation-bgp-left-deep-reordering/lib/ActorQueryOperationBgpLeftDeepReordering.ts

+        cluster.variables.filter((v) => vars.findIndex((val) => val.equals(v)) < 0)), [])
+      .map((term) => termToString(term));
+
+    return { type: 'bindings', bindingsStream, variables };


So this actor doesn't provide metadata?

I know it's not required, but it might be good to provide an estimate if possible.

Originally I didn't do it because it would require calling the metadata from all patterns not the cluster. But now that metadata is in a separate function I will add it. I do wonder how useful this value is going to be for anyone though. :D

joachimvh · 2018-03-08T08:39:03Z

Going to rerun the evaluations with this algorithm, but my guess is that it will sometimes be faster and sometimes slower (as the current comparison results with the original TPF client are). But how to determine which one is faster is not really feasible I think. If you can do that you could probably solve a lot of problems for query algorithms :D

rubensworks · 2018-03-14T00:46:54Z

@joachimvh Should this be merged as well? Or do you want to evaluate or make some changes first?

joachimvh · 2018-03-14T08:18:18Z

I'm currently re-running the evaluations on a separate server to see if the performance is really that bad compared to the others. I'll see after that. Not sure if it is worth merging if the performance is terrible anyway.

joachimvh · 2018-03-14T12:27:08Z

Reran the tests. For some reason this actor performs well in the beginning but keeps getting worse results the longer the longer the evaluation goes on. Will have to check what's up with that.

rubensworks · 2018-03-14T23:56:54Z

That's strange. Perhaps a stream that doesn't get closed and keeps running for some reason, but I wouldn't know this would occur, everything looks fine to me.

joachimvh · 2018-03-15T08:30:18Z

Yea, will have to look in to how to debug this, apparently I managed to create a memory leak in Comunica I guess :P. It's quite consistent though, did another run on the evaluation and had similar results results, first iteration needs 15s for C1 while the last one needs 50+

rubensworks · 2018-03-16T01:07:19Z

@joachimvh Should this be included in release 1.0.0 still?

joachimvh · 2018-03-16T08:27:29Z

I would say no, unless I manage to fix the "memory leak" thing by this evening.

joachimvh · 2018-03-16T15:55:43Z

Spent some more time on this. Still no idea what the problem is. :|
I did discover that removing the metadat function entirely sped up performance by 10% which surprised me.

rubensworks · 2018-03-19T22:59:25Z

@joachimvh For reference, some changes were made yesterday to the README and package.json templates. Before this is merged, we should update those in this PR as well. (not urgent)

rubensworks · 2018-06-13T06:49:08Z

@joachimvh What should we do with this one? Fix it up so that we can merge it? Or close it?

joachimvh · 2018-06-13T07:33:07Z

Well preferably we should fix it, problem is I don't know what the issue is. :D Should check in if it is magically fixed at this point by all the changes that happened since then. Just closing it seems weird since it should be no problem to do this with Comunica.

rubensworks · 2020-07-23T08:45:42Z

I'm closing this PR due to staleness. The branch still exists should we want to fix it in the future.

joachimvh added the feature ➕ label Mar 7, 2018

joachimvh added this to To Do in Development 1.0.0 via automation Mar 7, 2018

joachimvh requested a review from rubensworks March 7, 2018 16:34

joachimvh moved this from To Do to To Review in Development 1.0.0 Mar 7, 2018

joachimvh self-assigned this Mar 7, 2018

rubensworks reviewed Mar 7, 2018

View reviewed changes

joachimvh added 3 commits March 8, 2018 09:44

Added BGP actor using the original LDF algorithm

0f4d4a0

Execute all promises at the same time for speedup

42a8d19

Update to new metadata structure

8d47370

joachimvh force-pushed the feature/bgp-left-deep-reordering branch from 3ec1f5e to 04c776b Compare March 8, 2018 11:57

Added metadata function and autostart streams

7d9d3c6

joachimvh force-pushed the feature/bgp-left-deep-reordering branch from 04c776b to 7d9d3c6 Compare March 8, 2018 12:23

rubensworks mentioned this pull request Mar 13, 2018

Add BGP left-deep-smallest-sort Query Operation Actor #111

Merged

rubensworks mentioned this pull request Mar 14, 2018

Reorder subpatterns in BGP #58

Closed

rubensworks added this to To Do in Development Major via automation Mar 19, 2018

rubensworks removed this from To Review in Development 1.0.0 Mar 19, 2018

rubensworks moved this from To Do to In Progress in Development Major Mar 19, 2018

rubensworks force-pushed the master branch 2 times, most recently from 0cc8431 to abcbd8e Compare June 21, 2018 09:56

rubensworks force-pushed the master branch 9 times, most recently from 88cec75 to ba7bf64 Compare June 29, 2018 13:02

rubensworks force-pushed the master branch 2 times, most recently from 2a7430b to 6404c21 Compare October 5, 2018 08:42

rubensworks force-pushed the master branch from 66d83bb to 6404c21 Compare October 30, 2018 10:17

rubensworks added the in-progress label Nov 29, 2018

rubensworks closed this Jul 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/bgp left deep reordering #102

Feature/bgp left deep reordering #102

joachimvh commented Mar 7, 2018

coveralls commented Mar 7, 2018 •

edited

Loading

rubensworks Mar 7, 2018

joachimvh Mar 8, 2018

rubensworks left a comment

rubensworks Mar 7, 2018

joachimvh Mar 8, 2018

rubensworks Mar 9, 2018

rubensworks Mar 7, 2018

rubensworks Mar 7, 2018

rubensworks Mar 7, 2018

joachimvh Mar 8, 2018

joachimvh commented Mar 8, 2018

rubensworks commented Mar 14, 2018

joachimvh commented Mar 14, 2018

joachimvh commented Mar 14, 2018

rubensworks commented Mar 14, 2018

joachimvh commented Mar 15, 2018

rubensworks commented Mar 16, 2018

joachimvh commented Mar 16, 2018

joachimvh commented Mar 16, 2018

rubensworks commented Mar 19, 2018

rubensworks commented Jun 13, 2018

joachimvh commented Jun 13, 2018

rubensworks commented Jul 23, 2020

Feature/bgp left deep reordering #102

Feature/bgp left deep reordering #102

Conversation

joachimvh commented Mar 7, 2018

coveralls commented Mar 7, 2018 • edited Loading

Pull Request Test Coverage Report for Build 49

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubensworks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joachimvh commented Mar 8, 2018

rubensworks commented Mar 14, 2018

joachimvh commented Mar 14, 2018

joachimvh commented Mar 14, 2018

rubensworks commented Mar 14, 2018

joachimvh commented Mar 15, 2018

rubensworks commented Mar 16, 2018

joachimvh commented Mar 16, 2018

joachimvh commented Mar 16, 2018

rubensworks commented Mar 19, 2018

rubensworks commented Jun 13, 2018

joachimvh commented Jun 13, 2018

rubensworks commented Jul 23, 2020

coveralls commented Mar 7, 2018 •

edited

Loading