-
Notifications
You must be signed in to change notification settings - Fork 49
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
Conversation
There was a problem hiding this 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.
- The repository seems missing the actual code. i.e. the
neptune-graph-explorer/
folder. - Is it necessary to upload the code for all the node_modules?
There was a problem hiding this 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.
There was a problem hiding this 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/components/DatePickerInput/DatePickerInput.tsx
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/edgeLabelsTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/incomingNeighborsCountTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/incomingNeighborsCountTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/incomingNeighborsCountTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/incomingOneHopTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/incomingOneHopTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/incomingOneHopTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/incomingOneHopTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/outgoingNeighborsCountTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/edgeLabelsTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/incomingOneHopTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/edgeLabelsTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/vertexLabelsTemplate.ts
Outdated
Show resolved
Hide resolved
packages/client/src/connector/sparQL/templates/vertexSchemaTemplate.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
* g.V("124") | ||
* .project("vertices", "edges") | ||
* .by( | ||
* both().hasLabel("airport").dedup().range(0,10).fold() | ||
* ) | ||
* .by( | ||
* bothE("route").dedup().range(0,10).fold() | ||
* ) |
There was a problem hiding this comment.
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]]
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]}]
There was a problem hiding this comment.
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?
* 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() | ||
* ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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.
There was a problem hiding this 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.
- Change sparQL to sparql or SPARQL.
- SPARQL queries should be signed off by Charles.
12eedca
There was a problem hiding this 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?
There was a problem hiding this 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.
…guration Agutierrez/add default configuration
This merges in the workbench updates as of 11/07/22 from Bitbucket to Github.