Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

11/07 graph-explorer #1

Merged
merged 15 commits into from
Nov 28, 2022
Merged

11/07 graph-explorer #1

merged 15 commits into from
Nov 28, 2022

Conversation

jackson-millard
Copy link
Contributor

This merges in the workbench updates as of 11/07/22 from Bitbucket to Github.

Copy link

@srondelli srondelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, thanks for sharing the code.

  1. The repository seems missing the actual code. i.e. the neptune-graph-explorer/ folder.
  2. Is it necessary to upload the code for all the node_modules?

Copy link

@srondelli srondelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave some comments to the README. Going to take a look to the code. Note: I haven;t actually tried to follow it yet (i.e. checkout the code and build and run), once I do that i may have more comments.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link

@srondelli srondelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got just at the beginning of SPARQL. Will continue later this week.

packages/client/src/@types/entities.ts Outdated Show resolved Hide resolved
packages/client/src/@types/entities.ts Outdated Show resolved Hide resolved
packages/client/src/@types/entities.ts Outdated Show resolved Hide resolved
packages/client/src/@types/entities.ts Outdated Show resolved Hide resolved
packages/client/src/connector/AbstractConnector.ts Outdated Show resolved Hide resolved
packages/client/src/connector/auth/RequestSig.ts Outdated Show resolved Hide resolved
packages/client/src/connector/auth/RequestSig.ts Outdated Show resolved Hide resolved
packages/client/src/connector/sparQL/SparQLConnector.ts Outdated Show resolved Hide resolved
packages/client/src/connector/auth/RequestSig.ts Outdated Show resolved Hide resolved
packages/client/src/hooks/useDeepMemo.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
packages/client/src/utils/date/formatDate.ts Outdated Show resolved Hide resolved
packages/client/src/utils/formatDate.ts Show resolved Hide resolved
packages/client/src/reportWebVitals.ts Outdated Show resolved Hide resolved
@joywa joywa self-requested a review November 16, 2022 20:57
@joywa joywa changed the title 11/07 Workbench Update 11/07 graph-explorer Nov 16, 2022
README.md Outdated Show resolved Hide resolved
Copy link

@gopuneet gopuneet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the proxy-server code and added comments.

packages/proxy-server/node-server.js Show resolved Hide resolved
packages/proxy-server/package.json Outdated Show resolved Hide resolved
packages/proxy-server/node-server.js Outdated Show resolved Hide resolved
packages/client/.env.production Outdated Show resolved Hide resolved
packages/proxy-server/node-server.js Show resolved Hide resolved
packages/proxy-server/node-server.js Outdated Show resolved Hide resolved
Comment on lines +97 to +104
* g.V("124")
* .project("vertices", "edges")
* .by(
* both().hasLabel("airport").dedup().range(0,10).fold()
* )
* .by(
* bothE("route").dedup().range(0,10).fold()
* )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This query is fine for a single hop and fetching smaller set of results, but a more efficient query to fetch 1 hop vertices and edges could be:

g.V("1")
.bothE("knows").otherV().hasLabel("person").dedup().range(0,10)
.path()

This would produce the below path results for each solution. Each solution is a path containing the starting vertex, an edge and the 1-hop vertex.

path[v[1], e[7][1-knows->2], v[2]]
path[v[1], e[8][1-knows->4], v[4]]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nestoralvarezd could you please take a look?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed the above query. The .path() chain in the query give me the information of the relation and the id of the neighbor. However, in the original query, I'm getting the properties of each edge and node which is related to the searched node. So, this change will imply doing 1 extra query by each resulting node.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original query might be returning properties because of the serializer being used. It is best to always query for the exact data needed for each vertex/edge instead of relying of serialization.

In case properties, id and label info is needed for vertices and edges on the path, you could get them as maps in the path using by modulation on the last path step:

g.V("1").bothE("knows").otherV().hasLabel("person").dedup().range(0,10)
.path().by(valueMap().with(WithOptions.tokens,WithOptions.all))

This would return results as below:

path[{<T.label: 4>: 'person', 'name': ['marko'], <T.id: 1>: '1', 'age': [29]}, {<T.label: 4>: 'knows', 'weight': 0.5, <T.id: 1>: '7'}, {<T.label: 4>: 'person', 'name': ['vadas'], <T.id: 1>: '2', 'age': [27]}]

path[{<T.label: 4>: 'person', 'name': ['marko'], <T.id: 1>: '1', 'age': [29]}, {<T.label: 4>: 'knows', 'weight': 1.0, <T.id: 1>: '8'}, {<T.label: 4>: 'person', 'name': ['josh'], <T.id: 1>: '4', 'age': [32]}]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed this version and I believe that it'll work as expected. Do you want to update the queries in a new PR?

Comment on lines +116 to +126
* g.V("124")
* .project("vertices", "edges")
* .by(
* both().hasLabel("airport").and(
* has("longest", gt(10000)),
* has("country", containing("ES"))
* ).dedup().range(0,10).fold()
* )
* .by(
* bothE("route").dedup().fold()
* )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be written similar to the above query, but should be ok for a smaller set of results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nestoralvarezd could you please take a look?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same answer here, .path() works if I want to only know the relation and its attributes. However, I'm fetching the properties as well

saikiranboga
saikiranboga previously approved these changes Nov 23, 2022
Copy link

@saikiranboga saikiranboga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed Gremlin queries and mostly look good to me. Added couple of optional suggestions.

srondelli
srondelli previously approved these changes Nov 23, 2022
Copy link

@srondelli srondelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for all the changes.

  1. Change sparQL to sparql or SPARQL.
  2. SPARQL queries should be signed off by Charles.

packages/client/index.html Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@joywa joywa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackson-millard-experoinc thanks, this commit looks great. Can you make the small modifications I just left?

bechbd
bechbd previously approved these changes Nov 23, 2022
Copy link
Contributor

@joywa joywa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Will address bnode support + Gremlin optimizations in later PRs.

@michaelnchin michaelnchin merged commit 8a6eac7 into aws:main Nov 28, 2022
michaelnchin pushed a commit that referenced this pull request Aug 2, 2023
…guration

Agutierrez/add default configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

10 participants