Implement EXTENDS syntax for Graph DDL #787
Conversation
Co-authored-by: Sören Reichardt <[email protected]>
Co-authored-by: Sören Reichardt <[email protected]>
Co-authored-by: Sören Reichardt <[email protected]>
Co-authored-by: Sören Reichardt <[email protected]>
* access to all element types for recursive lookup Co-authored-by: Sören Reichardt <[email protected]>
Co-authored-by: Sören Reichardt <[email protected]>
Co-authored-by: Sören Reichardt <[email protected]>
Co-authored-by: Sören Reichardt <[email protected]>
Co-authored-by: Sören Reichardt <[email protected]>
Co-authored-by: Sören Reichardt <[email protected]>
|
Quick turnaround on that one! |
|
|
||
| def elementTypeDefinition[_: P]: P[ElementTypeDefinition] = | ||
| P(identifier.! ~/ extendsDefinition.?.map(_.getOrElse(Set.empty)) ~/ properties.?.map(_.getOrElse(Map.empty)) ~/ keyDefinition.?).map { | ||
| case (id, parents, props, maybeKey) => ElementTypeDefinition(id, parents, props, maybeKey) |
There was a problem hiding this comment.
Maybe we could move the map(_.getOrElse(???)) stuff inside the case in order to make the parser it self more easily readable
| GraphDdl(ddl).graphs(graphName).graphType should equal( | ||
| Schema.empty | ||
| .withNodePropertyKeys("A")("foo" -> CTString) | ||
| .withNodePropertyKeys("A", "B")("foo" -> CTString, "bar" -> CTString) |
There was a problem hiding this comment.
Seeing both definitions side by side the result is rather surprising.
| } | ||
| resolved.head.name | ||
| } | ||
|
|
||
| private def mergeProperties(elementTypes: Set[ElementTypeDefinition]): PropertyKeys = { | ||
| elementTypes | ||
| .flatMap(_.properties) | ||
| .foldLeft(PropertyKeys.empty) { case (props, (name, cypherType)) => | ||
| props.get(name).filter(_ != cypherType) match { | ||
| case Some(t) => incompatibleTypes(name, cypherType, t) |
There was a problem hiding this comment.
Side note:
Why do we throw an error here? Shouldn't Cypher generally support this. It is more a spark-cypher restriction
There was a problem hiding this comment.
Exactly! I thought the same and will change it in another PR and do the error handling in SQL PGDS (just didn't want to touch it in this PR)
| ) | ||
| ) | ||
|
|
||
| it("allows compact inline graph definition with complex node type") { |
There was a problem hiding this comment.
Could we add a test for conflicting property types, e.g.
A (y String)
B (y Integer)
C EXTENDS A, B
There was a problem hiding this comment.
This has been tested before for the non-extends variant. However, I added an additional test.
| private def resolveParents(node: ElementTypeDefinition): Set[ElementTypeDefinition] = | ||
| node.parents.map(resolveElementType).flatMap(resolveParents) + node | ||
|
|
||
| private def detectCircularDependency(node: ElementTypeDefinition): Unit = { |
This introduces
EXTENDSsyntax as in the following example:We still support the previous syntax, e.g. given the above element types it is valid to write: