Unstable neo4j update #388

LaraMerdol · 2023-10-09T09:56:59Z

I have made changes to the visual application in response to the issues mentioned in #387. The modifications involve using Neo4j version 5.10. Here are the details of the changes made:

The APOC procedure 'apoc.cypher.runTimeboxed' has been deprecated in Neo4j 4 and later versions. Since there is no direct replacement with a built-in timeout parameter in the newer versions of APOC, I have used 'apoc.cypher.run' and implemented a timeout mechanism inside the 'neo4j-db.service.ts'.
The function ID() is deprecated and it is recommended to use elementID() instead. To solve this problem, I have used elementId instead of id. The elementID is a string type and is structured as "4:c0a65d96-4993-4b0c-b036-e7ebd9174905:0". However, this type of id is not a proper selector id because it is also used as a CSS selector. To overcome this problem, I converted the colon (:) to an underscore (_) to create a valid CSS selector character sequence. For example, "4:c0a65d96-4993-4b0c-b036-e7ebd9174905:0" becomes "n4_c0a65d96-4993-4b0c-b036-e7ebd9174905_0".
The default minimum password length is 8 characters, which is a requirement introduced in Neo4j 5.3.

For testing on your local environment, you can use the new Neo4j 5.10 database working on the ivis server port:3001. The credentials are username: neo4j and password: 12345678. You should change the environment.ts accordingly.

… to 4500 also fix database port

canbax · 2023-10-15T18:43:41Z

However, this type of id is not a proper selector id because it is also used as a CSS selector

You mean it is not a valid cytoscape.js selector. Right? If so where does these selectors are used? As far as I remember, they are only used in highlighting elements on hover. If it's the case, why don't we select them with selectors like [id = 'foo'] as stated https://js.cytoscape.org/#selectors/group-class-amp-id

Always replacing elementIds seems high maintenance to me. Also I see that elementId is just a string and it can be a cytoscape.js element id.

LaraMerdol · 2023-10-17T07:26:56Z

However, this type of id is not a proper selector id because it is also used as a CSS selector

You mean it is not a valid cytoscape.js selector. Right? If so where does these selectors are used? As far as I remember, they are only used in highlighting elements on hover. If it's the case, why don't we select them with selectors like [id = 'foo'] as stated https://js.cytoscape.org/#selectors/group-class-amp-id

Always replacing elementIds seems high maintenance to me. Also I see that elementId is just a string and it can be a cytoscape.js element id.

Yes, you are right. I changed the selectors as you suggested. Now all elementIds are in the same format without any replacements used.

canbax

1-) In some places, there might be some performance regression. O(n) is replaced with O(n^2). I think those can be done in O(n)

2-) I don't have access to deployed visual env. Please test there

3-) some time ago, there were Cypress e2e tests. Do they still run? If so, can you run them

4-) As an improvement those Cypress test could be run as a new step of the current Github Action

If you do points 1 & 2 I think it's good to merge. 3 & 4 could be future improvements. But if you could do point 3, it would be great to see this big change doesn't cause any bugs.

canbax · 2023-10-18T10:21:49Z

src/app/visuall/cytoscape.service.ts

@@ -202,8 +201,7 @@ export class CytoscapeService {
    let compoundEdgeIds2 = this._g.cy.edges('.' + C.COLLAPSED_EDGE_CLASS).map(x => x.id());
    elemIds.push(...C.arrayDiff(compoundEdgeIds, compoundEdgeIds2));
    // elements might already exist but hidden, so show them
-    this._g.viewUtils.show(this._g.cy.$(elemIds.map(x => '#' + x).join(',')));
-
+    this._g.viewUtils.show(this._g.cy.nodes().filter(node => elemIds.includes(node.id())));


issue: includes inside .filter() function might be O(n^2). The previous implementation might be faster. Please check this.

It is no longer possible to use the previous implementation as the ID cannot be utilized as a CSS selector anymore. Your observation about the runtime being O(n^2) was correct, and this is not an optimal practice. To enhance runtime to O(n), we can use a set to gather element IDs. In a set, the 'has' operation requires constant time due to the hash table implementation, but it also results in unnecessary memory consumption.

const elemIdSet = new Set(elemIds); this._g.viewUtils.show(this._g.cy.nodes().filter(node => elemIdSet.has(node.id())));

src/app/visuall/cytoscape.service.ts

canbax · 2023-10-18T10:25:16Z

src/app/visuall/db-service/neo4j-db.service.ts

-    if (!isTimeboxed) {
-      q = query;
-    }
+    let q = isTimeboxed


praise: good that if removed. q might be a const

canbax · 2023-10-18T10:29:13Z

src/app/visuall/operation-tabs/query-tab/advanced-queries/advanced-queries.component.ts

@@ -296,7 +296,7 @@ export class AdvancedQueriesComponent implements OnInit, OnDestroy {

  private highlightSeedNodes() {
    const dbIds = this.selectedNodes.map(x => x.dbId);
-    const seedNodes = this._g.cy.nodes(dbIds.map(x => '#n' + x).join());
+    const seedNodes = this._g.cy.nodes().filter(node => dbIds.includes(node.id().substring(1)));


issue: .includes inside .filter might be O(n^2). This is a performance regression. Can we do it in linear time?

canbax · 2023-10-18T10:29:32Z

src/app/visuall/operation-tabs/query-tab/advanced-queries/advanced-queries.component.ts

@@ -316,7 +316,7 @@ export class AdvancedQueriesComponent implements OnInit, OnDestroy {
    if (!dbIds || dbIds.length < 1) {
      return;
    }
-    const cyNodes = this._g.cy.nodes(dbIds.map(x => '#n' + x).join());
+    const cyNodes = this._g.cy.nodes().filter(node => dbIds.includes(node.id().substring(1)));


issue: .includes inside .filter might be O(n^2). This is a performance regression. Can we do it in linear time?

src/environments/environment.prod.ts

src/app/visuall/operation-tabs/query-tab/advanced-queries/advanced-queries.component.ts

LaraMerdol · 2023-10-18T15:14:07Z

1-) In some places, there might be some performance regression. O(n) is replaced with O(n^2). I think those can be done in O(n)

2-) I don't have access to deployed visual env. Please test there

3-) some time ago, there were Cypress e2e tests. Do they still run? If so, can you run them

4-) As an improvement those Cypress test could be run as a new step of the current Github Action

If you do points 1 & 2 I think it's good to merge. 3 & 4 could be future improvements. But if you could do point 3, it would be great to see this big change doesn't cause any bugs.

I'd like to inform you of the following updates:

I've come up with an alternative solution that can enhance the run time to O(n).
I've tested on my local environment. If you want to test it on your local environment, you can use the new Neo4j 5.10 database running on the ivis server port:3001. The credentials to log in are username: neo4j and password: 12345678. You should also update the environment.ts file accordingly.
Unfortunately, I'm unable to run Cypress e2e tests as it keeps giving me a 503: Service Unavailable error.

canbax

I don't see any other issue. I don't have access to Visuall env. So I cannot test it properly with the deployed environment. But it looks OK except for the issue I mentioned

canbax · 2023-10-20T16:03:44Z

src/app/visuall/cytoscape.service.ts

-    this._g.viewUtils.show(this._g.cy.$(elemIds.map(x => '#' + x).join(',')));
-
+    const elemIdSet = new Set(elemIds);
+    this._g.viewUtils.show(this._g.cy.nodes().filter(node => elemIdSet.has(node.id())));


issue: elemId could be an edge. So don't use cy.nodes()instead use cy.elements()

LaraMerdol added 6 commits September 9, 2023 16:14

Fix issue #387 : Database version update

e7c93bd

Fix issue #387: İd's were changed with elementId

c6dddab

Change the port of the visuall in server from 4200 (ocupied by hydra)…

d6f92e8

… to 4500 also fix database port

Change the port of the visuall in server from 4200 (ocupied by hydra)…

4cd33a1

… to 4500 also fix database port

Fix issue #387: minor changes

7af6a99

Fix issue #387:Minor change

eeddabb

LaraMerdol added 2 commits October 16, 2023 23:54

Fix issue #387: cytoscape selectors changed

06aca0b

Fix issue #387: minor change

4ed40ee

canbax requested changes Oct 18, 2023

View reviewed changes

LaraMerdol added 2 commits October 18, 2023 17:02

Fix issue #387: unnecessary assignment operatıon removed

8fad035

Fix issue #387: o(n^2) implementation improved by using set operation

776dc83

LaraMerdol commented Oct 18, 2023

View reviewed changes

src/app/visuall/operation-tabs/query-tab/advanced-queries/advanced-queries.component.ts Show resolved Hide resolved

LaraMerdol requested a review from canbax October 18, 2023 15:14

canbax approved these changes Oct 20, 2023

View reviewed changes

Fix issue #387:Fix nodes to elements

a4f4de3

LaraMerdol closed this Oct 23, 2023

LaraMerdol deleted the unstable-neo4j-update branch October 23, 2023 08:04

LaraMerdol restored the unstable-neo4j-update branch October 23, 2023 11:37

LaraMerdol reopened this Oct 23, 2023

LaraMerdol closed this Oct 23, 2023

LaraMerdol deleted the unstable-neo4j-update branch October 23, 2023 11:38

LaraMerdol mentioned this pull request Oct 23, 2023

Database version update #387

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unstable neo4j update #388

Unstable neo4j update #388

LaraMerdol commented Oct 9, 2023 •

edited

Loading

canbax commented Oct 15, 2023 •

edited

Loading

LaraMerdol commented Oct 17, 2023

canbax left a comment

canbax Oct 18, 2023

LaraMerdol Oct 18, 2023

canbax Oct 18, 2023

canbax Oct 18, 2023

canbax Oct 18, 2023

LaraMerdol commented Oct 18, 2023

canbax left a comment

canbax Oct 20, 2023

Unstable neo4j update #388

Unstable neo4j update #388

Conversation

LaraMerdol commented Oct 9, 2023 • edited Loading

canbax commented Oct 15, 2023 • edited Loading

LaraMerdol commented Oct 17, 2023

canbax left a comment

Choose a reason for hiding this comment

canbax Oct 18, 2023

Choose a reason for hiding this comment

LaraMerdol Oct 18, 2023

Choose a reason for hiding this comment

canbax Oct 18, 2023

Choose a reason for hiding this comment

canbax Oct 18, 2023

Choose a reason for hiding this comment

canbax Oct 18, 2023

Choose a reason for hiding this comment

LaraMerdol commented Oct 18, 2023

canbax left a comment

Choose a reason for hiding this comment

canbax Oct 20, 2023

Choose a reason for hiding this comment

LaraMerdol commented Oct 9, 2023 •

edited

Loading

canbax commented Oct 15, 2023 •

edited

Loading