Skip to content

Commit

Permalink
Optimisation: Remove redundant relationship validation (typedb#2126)
Browse files Browse the repository at this point in the history 
* Benchmarks for adding relation

* Replace low level test with a clearer higher level one

* WIP

* Get rid of redudent validation rule

* Some missing refactors

* Killing the goat
  • Loading branch information
@fppt @aelred
fppt authored and aelred committed Sep 22, 2017
1 parent 78d544a commit d35175a
Show file tree
Hide file tree
Showing 9 changed files with 123 additions and 103 deletions.
92 changes: 52 additions & 40 deletions grakn-kb/src/main/java/ai/grakn/kb/internal/ValidateGlobalRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@
import static ai.grakn.util.ErrorMessage.VALIDATION_NOT_EXACTLY_ONE_KEY;
import static ai.grakn.util.ErrorMessage.VALIDATION_RELATION_CASTING_LOOP_FAIL;
import static ai.grakn.util.ErrorMessage.VALIDATION_RELATION_DUPLICATE;
import static ai.grakn.util.ErrorMessage.VALIDATION_RELATION_MORE_CASTING_THAN_ROLES;
import static ai.grakn.util.ErrorMessage.VALIDATION_RELATION_TYPE;
import static ai.grakn.util.ErrorMessage.VALIDATION_RELATION_TYPES_ROLES_SCHEMA;
import static ai.grakn.util.ErrorMessage.VALIDATION_REQUIRED_RELATION;
Expand Down Expand Up @@ -98,19 +97,61 @@ private ValidateGlobalRules() {
}

/**
* This method checks if the plays edge has been added successfully. It does so By checking
* Casting -CAST-> ConceptInstance -ISA-> Concept -PLAYS-> X =
* Casting -ISA-> X
* @param casting The casting to be validated
* @return A specific error if one is found.
* This method checks if the plays edge has been added between the roleplayer's {@link Type} and
* the {@link Role} being played.
*
* It also checks if the {@link Role} of the {@link Casting} has been linked to the {@link RelationshipType} of the
* {@link Relationship} which the {@link Casting} connects to.
*
* @return Specific errors if any are found
*/
static Set<String> validatePlaysAndRelatesStructure(Casting casting) {
Set<String> errors = new HashSet<>();

//Gets here to make sure we traverse/read only once
Thing thing = casting.getRolePlayer();
Role role = casting.getRole();
Relationship relation = casting.getRelationship();

//Actual checks
roleNotAllowedToBePlayed(role, thing).ifPresent(errors::add);
roleNotLinkedToRelationShip(role, relation.type(), relation).ifPresent(errors::add);

return errors;
}

/**
* Checks if the {@link Role} of the {@link Casting} has been linked to the {@link RelationshipType} of
* the {@link Relationship} which the {@link Casting} connects to.
*
* @param role the {@link Role} which the {@link Casting} refers to
* @param relationshipType the {@link RelationshipType} which should connect to the role
* @param relationship the {@link Relationship} which the {@link Casting} refers to
* @return an error if one is found
*/
static Optional<String> validatePlaysStructure(Casting casting) {
Thing thing = casting.getInstance();
private static Optional<String> roleNotLinkedToRelationShip(Role role, RelationshipType relationshipType, Relationship relationship){
boolean notFound = role.relationshipTypes().
noneMatch(innerRelationType -> innerRelationType.getLabel().equals(relationshipType.getLabel()));
if(notFound){
return Optional.of(VALIDATION_RELATION_CASTING_LOOP_FAIL.getMessage(relationship.getId(), role.getLabel(), relationshipType.getLabel()));
}
return Optional.empty();
}

/**
* Checks if the plays edge has been added between the roleplayer's {@link Type} and
* the {@link Role} being played.
*
* Also checks that required {@link Role} are satisfied
*
* @param role The {@link Role} which the role-player is playing
* @param thing the role-player
* @return an error if one is found
*/
private static Optional<String> roleNotAllowedToBePlayed(Role role, Thing thing){
TypeImpl<?, ?> currentConcept = (TypeImpl<?, ?>) thing.type();
Role role = casting.getRoleType();

boolean satisfiesPlays = false;

while(currentConcept != null){
Map<Role, Boolean> plays = currentConcept.directPlays();

Expand All @@ -132,7 +173,7 @@ static Optional<String> validatePlaysStructure(Casting casting) {
if(satisfiesPlays) {
return Optional.empty();
} else {
return Optional.of(VALIDATION_CASTING.getMessage(thing.type().getLabel(), thing.getId(), casting.getRoleType().getLabel()));
return Optional.of(VALIDATION_CASTING.getMessage(thing.type().getLabel(), thing.getId(), role.getLabel()));
}
}

Expand Down Expand Up @@ -161,35 +202,6 @@ static Optional<String> validateHasMinimumRoles(RelationshipType relationshipTyp
}
}

/**
*
* @param relation The assertion to validate
* @return An error message indicating if the relation has an incorrect structure. This includes checking if there an equal
* number of castings and roles as well as looping the structure to make sure castings lead to the same relation type.
*/
static Optional<String> validateRelationshipStructure(RelationshipReified relation){
RelationshipType relationshipType = relation.type();
Collection<Casting> castings = relation.castingsRelation().collect(Collectors.toSet());
Collection<Role> roles = relationshipType.relates().collect(Collectors.toSet());

Set<Role> rolesViaRolePlayers = castings.stream().map(Casting::getRoleType).collect(Collectors.toSet());

if(rolesViaRolePlayers.size() > roles.size()) {
return Optional.of(VALIDATION_RELATION_MORE_CASTING_THAN_ROLES.getMessage(relation.getId(), rolesViaRolePlayers.size(), relationshipType.getLabel(), roles.size()));
}

for(Casting casting : castings){
boolean notFound = casting.getRoleType().relationshipTypes().
noneMatch(innerRelationType -> innerRelationType.getLabel().equals(relationshipType.getLabel()));

if(notFound) {
return Optional.of(VALIDATION_RELATION_CASTING_LOOP_FAIL.getMessage(relation.getId(), casting.getRoleType().getLabel(), relationshipType.getLabel()));
}
}

return Optional.empty();
}

/**
*
* @param relationshipType the relation type to be validated
Expand Down
3 changes: 1 addition & 2 deletions grakn-kb/src/main/java/ai/grakn/kb/internal/Validator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ private void validateRelation(GraknTxAbstract<?> graph, Relationship relationshi
Optional<RelationshipReified> relationReified = ((RelationshipImpl) relationship).reified();
//TODO: We need new validation mechanisms for non-reified relations
relationReified.ifPresent(relationReified1 -> {
ValidateGlobalRules.validateRelationshipStructure(relationReified1).ifPresent(errorsFound::add);
ValidateGlobalRules.validateRelationIsUnique(graph, relationReified1).ifPresent(errorsFound::add);
});
}
Expand All @@ -127,7 +126,7 @@ private void validateRelation(GraknTxAbstract<?> graph, Relationship relationshi
* @param casting The Role player to validate
*/
private void validateCasting(Casting casting){
ValidateGlobalRules.validatePlaysStructure(casting).ifPresent(errorsFound::add);
errorsFound.addAll(ValidateGlobalRules.validatePlaysAndRelatesStructure(casting));
}

/**
Expand Down
4 changes: 2 additions & 2 deletions grakn-kb/src/main/java/ai/grakn/kb/internal/concept/RelationshipReified.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,13 @@ public Map<Role, Set<Thing>> allRolePlayers() {

//We add the role types explicitly so we can return them when there are no roleplayers
type().relates().forEach(roleType -> roleMap.put(roleType, new HashSet<>()));
castingsRelation().forEach(rp -> roleMap.computeIfAbsent(rp.getRoleType(), (k) -> new HashSet<>()).add(rp.getInstance()));
castingsRelation().forEach(rp -> roleMap.computeIfAbsent(rp.getRole(), (k) -> new HashSet<>()).add(rp.getRolePlayer()));

return roleMap;
}

public Stream<Thing> rolePlayers(Role... roles) {
return castingsRelation(roles).map(Casting::getInstance);
return castingsRelation(roles).map(Casting::getRolePlayer);
}

public void addRolePlayer(Role role, Thing thing) {
Expand Down
4 changes: 2 additions & 2 deletions grakn-kb/src/main/java/ai/grakn/kb/internal/concept/ThingImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ private void track(){
*/
@Override
public void delete() {
Set<Relationship> relationships = castingsInstance().map(Casting::getRelation).collect(Collectors.toSet());
Set<Relationship> relationships = castingsInstance().map(Casting::getRelationship).collect(Collectors.toSet());

vertex().tx().txCache().removedInstance(type().getId());
deleteNode();
Expand Down Expand Up @@ -243,7 +243,7 @@ private Stream<Relationship> edgeRelations(Role... roles){

@Override
public Stream<Role> plays() {
return castingsInstance().map(Casting::getRoleType);
return castingsInstance().map(Casting::getRole);
}

@Override
Expand Down
16 changes: 8 additions & 8 deletions grakn-kb/src/main/java/ai/grakn/kb/internal/structure/Casting.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,33 +66,33 @@ private EdgeElement edge(){

/**
*
* @return The role the instance is playing
* @return The {@link Role} the {@link Thing} is playing
*/
public Role getRoleType(){
public Role getRole(){
return cachedRoleType.get();
}

/**
*
* @return The relation type the instance is taking part in
* @return The {@link RelationshipType} the {@link Thing} is taking part in
*/
public RelationshipType getRelationType(){
public RelationshipType getRelationshipType(){
return cachedRelationType.get();
}

/**
*
* @return The relation which is linking the role and the instance
* @return The {@link Relationship} which is linking the {@link Role} and the instance
*/
public Relationship getRelation(){
public Relationship getRelationship(){
return cachedRelation.get();
}

/**
*
* @return The instance playing the role
* @return The {@link Thing} playing the {@link Role}
*/
public Thing getInstance(){
public Thing getRolePlayer(){
return cachedInstance.get();
}

Expand Down
46 changes: 9 additions & 37 deletions grakn-kb/src/test/java/ai/grakn/kb/internal/ValidateGlobalRulesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,25 +52,24 @@ public void testValidatePlaysStructure() throws Exception {
RelationshipImpl assertion = (RelationshipImpl) hunts.addRelationship().
addRolePlayer(witcher, geralt).addRolePlayer(monster, werewolf);
assertion.reified().get().castingsRelation().forEach(rolePlayer ->
assertTrue(ValidateGlobalRules.validatePlaysStructure(rolePlayer).isPresent()));
assertFalse(ValidateGlobalRules.validatePlaysAndRelatesStructure(rolePlayer).isEmpty()));

hunter.plays(witcher);

boolean [] flags = {false, false};
int count = 0;

for (Casting casting : assertion.reified().get().castingsRelation().collect(Collectors.toSet())) {
flags[count] = ValidateGlobalRules.validatePlaysStructure(casting).isPresent();
flags[count] = !ValidateGlobalRules.validatePlaysAndRelatesStructure(casting).isEmpty();
count++;
}
assertFalse(flags[0] && flags[1]);
assertTrue(flags[0] || flags[1]);
assertTrue(flags[0] && flags[1]);

wolf.sup(creature);
creature.plays(monster);

for (Casting casting : assertion.reified().get().castingsRelation().collect(Collectors.toSet())) {
assertFalse(ValidateGlobalRules.validatePlaysStructure(casting).isPresent());
assertFalse(ValidateGlobalRules.validatePlaysAndRelatesStructure(casting).isEmpty());
}
}

Expand All @@ -95,20 +94,20 @@ public void testValidatePlaysStructureUnique() {

// Valid with only a single relation
relation1.reified().get().castingsRelation().forEach(rolePlayer ->
assertFalse(ValidateGlobalRules.validatePlaysStructure(rolePlayer).isPresent()));
assertTrue(ValidateGlobalRules.validatePlaysAndRelatesStructure(rolePlayer).isEmpty()));

RelationshipImpl relation2 = (RelationshipImpl) relationshipType.addRelationship()
.addRolePlayer(role2, other2).addRolePlayer(role1, entity);

// Invalid with multiple relations
relation1.reified().get().castingsRelation().forEach(rolePlayer -> {
if (rolePlayer.getRoleType().equals(role1)) {
assertTrue(ValidateGlobalRules.validatePlaysStructure(rolePlayer).isPresent());
if (rolePlayer.getRole().equals(role1)) {
assertFalse(ValidateGlobalRules.validatePlaysAndRelatesStructure(rolePlayer).isEmpty());
}
});
relation2.reified().get().castingsRelation().forEach(rolePlayer -> {
if (rolePlayer.getRoleType().equals(role1)) {
assertTrue(ValidateGlobalRules.validatePlaysStructure(rolePlayer).isPresent());
if (rolePlayer.getRole().equals(role1)) {
assertFalse(ValidateGlobalRules.validatePlaysAndRelatesStructure(rolePlayer).isEmpty());
}
});
}
Expand All @@ -123,33 +122,6 @@ public void testValidateRelationTypeRelates() throws Exception {
assertFalse(ValidateGlobalRules.validateHasMinimumRoles(kills).isPresent());
}

@Test
public void testValidateAssertionStructure() throws Exception {
EntityType fakeType = tx.putEntityType("Fake Concept");
Role napper = tx.putRole("napper");
Role hunter = tx.putRole("hunter");
Role monster = tx.putRole("monster");
Role creature = tx.putRole("creature");
Thing cthulhu = fakeType.addEntity();
Thing werewolf = fakeType.addEntity();
Thing cartman = fakeType.addEntity();
RelationshipType kills = tx.putRelationshipType("kills");
RelationshipType naps = tx.putRelationshipType("naps").relates(napper);

RelationshipImpl assertion = (RelationshipImpl) kills.addRelationship().
addRolePlayer(hunter, cartman).addRolePlayer(monster, werewolf).addRolePlayer(creature, cthulhu);

kills.relates(monster);
assertTrue(ValidateGlobalRules.validateRelationshipStructure(assertion.reified().get()).isPresent());

kills.relates(hunter);
kills.relates(creature);
assertFalse(ValidateGlobalRules.validateRelationshipStructure(assertion.reified().get()).isPresent());

RelationshipImpl assertion2 = (RelationshipImpl) naps.addRelationship().addRolePlayer(hunter, cthulhu);
assertTrue(ValidateGlobalRules.validateRelationshipStructure(assertion2.reified().get()).isPresent());
}


@Test
public void testAbstractConceptValidation(){
Expand Down
18 changes: 18 additions & 0 deletions grakn-kb/src/test/java/ai/grakn/kb/internal/ValidatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,24 @@ public void whenCommittingNonAbstractRelationTypeNotLinkedToAnyRoleType_Throw(){
tx.commit();
}

@Test
public void whenCreatingRelationWithoutLinkingRelates_Throw(){
Role hunter = tx.putRole("hunter");
Role monster = tx.putRole("monster");
EntityType stuff = tx.putEntityType("Stuff").plays(hunter).plays(monster);
RelationshipType kills = tx.putRelationshipType("kills").relates(hunter);

Entity myHunter = stuff.addEntity();
Entity myMonster = stuff.addEntity();

Relationship relation = kills.addRelationship().addRolePlayer(hunter, myHunter).addRolePlayer(monster, myMonster);

expectedException.expect(InvalidKBException.class);
expectedException.expectMessage(containsString(ErrorMessage.VALIDATION_RELATION_CASTING_LOOP_FAIL.getMessage(relation.getId(), monster.getLabel(), kills.getLabel())));

tx.commit();
}

@Test
public void whenDeletingRelations_EnsureGraphRemainsValid() throws InvalidKBException {
// schema
Expand Down
16 changes: 8 additions & 8 deletions grakn-kb/src/test/java/ai/grakn/kb/internal/structure/CastingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@ public void whenCreatingRelation_EnsureRolePlayerContainsInstanceRoleTypeRelatio
Set<Casting> castings = relation.reified().get().castingsRelation().collect(Collectors.toSet());

castings.forEach(rolePlayer -> {
assertEquals(e1, rolePlayer.getInstance());
assertEquals(role1, rolePlayer.getRoleType());
assertEquals(relationshipType, rolePlayer.getRelationType());
assertEquals(relation, rolePlayer.getRelation());
assertEquals(e1, rolePlayer.getRolePlayer());
assertEquals(role1, rolePlayer.getRole());
assertEquals(relationshipType, rolePlayer.getRelationshipType());
assertEquals(relation, rolePlayer.getRelationship());
});
}

Expand All @@ -77,16 +77,16 @@ public void whenUpdatingRelation_EnsureRolePlayersAreUpdated(){
RelationshipImpl relation = (RelationshipImpl) relationshipType.addRelationship().
addRolePlayer(role1, e1);

Set<Thing> things = relation.reified().get().castingsRelation().map(Casting::getInstance).collect(Collectors.toSet());
Set<Role> roles = relation.reified().get().castingsRelation().map(Casting::getRoleType).collect(Collectors.toSet());
Set<Thing> things = relation.reified().get().castingsRelation().map(Casting::getRolePlayer).collect(Collectors.toSet());
Set<Role> roles = relation.reified().get().castingsRelation().map(Casting::getRole).collect(Collectors.toSet());
assertThat(things, containsInAnyOrder(e1));
assertThat(roles, containsInAnyOrder(role1));

//Now Update
relation.addRolePlayer(role2, e1).addRolePlayer(role3, e3);

things = relation.reified().get().castingsRelation().map(Casting::getInstance).collect(Collectors.toSet());
roles = relation.reified().get().castingsRelation().map(Casting::getRoleType).collect(Collectors.toSet());
things = relation.reified().get().castingsRelation().map(Casting::getRolePlayer).collect(Collectors.toSet());
roles = relation.reified().get().castingsRelation().map(Casting::getRole).collect(Collectors.toSet());
assertThat(things, containsInAnyOrder(e1, e3));
assertThat(roles, containsInAnyOrder(role1, role2, role3));
}
Expand Down
Loading

0 comments on commit d35175a

Please sign in to comment.