-
Notifications
You must be signed in to change notification settings - Fork 14
Schema based completions for nodes in paths #595
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
Conversation
|
| // and returns the "labels" of rels/nodes going out/in of the node/rel in the graph schema | ||
| function walkLabelTree( | ||
| relsFromLabels: Map<string, Set<string>>, | ||
| labelToConnectedLabelsMap: Map<string, Set<string>>, |
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.
We can use this identically if it is
Map<label, Set> (rel completion)
Map<rel types, Set>, (node completion)
I struggle to find a nice name that covers both. Maybe I should add the above comment into the code to help clarify things that way?
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.
In the grammar/database, they're just called "labels" right? Even if they can also be relationship types. It's not perfect, but at least somewhat consistent.
An idea for a better name for the labelToConnectedLabelsMap be to call it labelSegments ? I feel it makes it clear it's part of a path/patterns but not the full thing?
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.
Just a thought: is there a risk anywhere in our logic around people using the same names for a Reltype or label?
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 have some ideas on how we maybe could make this data structure better or more efficient, but I think we can wait until the feature matures to consider that refactoring
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.
About interference between rels/nodes with the same labels. Short answer - I see no risk
Longer:
We check for variable name/reference offset (really offset should be enough..) when fetching labels for a variable, so other variables will not interfere here -> the labels object on each symbol should be correct regardless of labels of other variables, even if the labels are the same.
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.
Regarding the naming. Im not sure of labelSegments... First thought is that seeing a collection called labelSegments I would think "list of segments", not map to list... but then if we use it like labelSegments.get(myLabel) I suppose that makes it clear that it's a map... I checked with ChatGPT and it proposed connectedLabelsMap, which I quite like. Or maybe even just connectedLabels
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'm happy to go with connectedLabels 👍
| { label: 'CATCHES', kind: CompletionItemKind.TypeParameter }, | ||
| { label: 'TRAINS', kind: CompletionItemKind.TypeParameter }, | ||
| { label: 'IS_IN', kind: CompletionItemKind.TypeParameter }, | ||
| //Limitation: Only take preceding node into account |
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.
If we keep slicing the query at the cursor we won't even have access to the fact that a "Pokemon" node follows the rel we are completing
| test('Simple node completion pattern', () => { | ||
| const query = 'MATCH (t:Trainer)-[r:CATCHES]->(:'; |
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.
It'd be nice to add a few more test cases (longer pattern, different relationships). Kind of like we have for the rel types?
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.
And for example showing tests of the limitations with regards to directionality
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 think directionality is covered by existing tests (like this one, you see the "limitation" comment inside), so im not sure if we need more tests that explicitly state the limitation?
| if (query.length > offset) { | ||
| query = query.slice(0, offset); | ||
| } |
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.
is this not done in the auto completion function now?
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.
Oh right, I fixed it here first when I thought id merge it in quickly, but then things came up and I did a separate PR with it.
Forgot to merge main to this, but you're right I should now remove this, good spot!
| // and returns the "labels" of rels/nodes going out/in of the node/rel in the graph schema | ||
| function walkLabelTree( | ||
| relsFromLabels: Map<string, Set<string>>, | ||
| labelToConnectedLabelsMap: Map<string, Set<string>>, |
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.
In the grammar/database, they're just called "labels" right? Even if they can also be relationship types. It's not perfect, but at least somewhat consistent.
An idea for a better name for the labelToConnectedLabelsMap be to call it labelSegments ? I feel it makes it clear it's part of a path/patterns but not the full thing?
| } | ||
|
|
||
| // limitation: not direction-aware (ignores <- vs ->) | ||
| // limitation: not checking relationship variable reuse |
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.
| // limitation: not checking relationship variable reuse | |
| // limitation: not checking node label repetition |
| // and returns the "labels" of rels/nodes going out/in of the node/rel in the graph schema | ||
| function walkLabelTree( | ||
| relsFromLabels: Map<string, Set<string>>, | ||
| labelToConnectedLabelsMap: Map<string, Set<string>>, |
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.
Just a thought: is there a risk anywhere in our logic around people using the same names for a Reltype or label?
| let currentRelEntry = nodesFromRelsSet.get(rel.relType); | ||
| if (!currentRelEntry) { | ||
| nodesFromRelsSet.set(rel.relType, new Set()); | ||
| currentRelEntry = nodesFromRelsSet.get(rel.relType); |
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 think it'd be cleaned to do a .has check first, and avoid the mutation of currentRelEntry
| // and returns the "labels" of rels/nodes going out/in of the node/rel in the graph schema | ||
| function walkLabelTree( | ||
| relsFromLabels: Map<string, Set<string>>, | ||
| labelToConnectedLabelsMap: Map<string, Set<string>>, |
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 have some ideas on how we maybe could make this data structure better or more efficient, but I think we can wait until the feature matures to consider that refactoring
OskarDamkjaer
left a comment
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.
Would be nice to have a simple undirected match test
MATCH (t:Trainer)-[r:CATCHES]-(:';and I think it could be good for a longer path as well
Implements similar logic that we used for paths like
MATCH (n:A)-[:to complete rels, to also work for node completions in paths like:MATCH (n:A)-[r:R]->(:Also uses the new
lastRuleproperty on the parsing result to get the latest node/rel pattern before the caret in both rel/node completions to avoid taking just "last pattern in parent ctx" failing on queries likeMATCH (n:A)-[r:R]->(:^)-[r2:R2]->(:Closes DEV-197