Skip to content

Commit

Permalink
Cleanup query exception handling (typedb#5091)
Browse files Browse the repository at this point in the history 
- removed the pattern validation from `QueryExecutor` -> we now validate queries using the `ReasonerQueryImpl::checkValid` facility
- we incorporate the fail-fast strategy, we throw an appropriate exception if we encounter any of the following:
  * query errors (mispelled labels, inexistent labels)
  * unmatched ids, illegal type combinations

This is different from the previous behaviour where we silently returned no results.

- we now correctly throw exceptions when dealing with illegal negation blocks
- an exception is thrown if inference is off and we execute a query with a negation block
  • Loading branch information
@kasper-piskorski
kasper-piskorski authored and Marco Scoppetta committed Apr 8, 2019
1 parent dd546ae commit c4b075d
Show file tree
Hide file tree
Showing 22 changed files with 309 additions and 253 deletions.
3 changes: 2 additions & 1 deletion common/exception/ErrorMessage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ public enum ErrorMessage {
//--------------------------------------------- Reasoner Errors -----------------------------------------------
NON_ATOMIC_QUERY("Addressed query is not atomic: [%s]."),
DISJUNCTIVE_NEGATION_BLOCK("Unsupported disjunction in a negation block."),
UNSAFE_NEGATION_BLOCK("Query:[%s] is not negation safe - negated pattern variables are not bound."),
UNSAFE_NEGATION_BLOCK("Query:\n[%s] is not negation safe - negated pattern variables are not bound."),
USING_NEGATION_WITH_REASONING_OFF("Query [%s] contains negation blocks. Please turn the reasoning on."),
NON_GROUND_NEQ_PREDICATE("Addressed query [%s] leads to a non-ground neq predicate when planning resolution."),
INCOMPLETE_RESOLUTION_PLAN("Addressed query [%s] leads to an incomplete resolution plan."),
ROLE_PATTERN_ABSENT("Addressed relation [%s] is missing a role pattern."),
Expand Down
16 changes: 8 additions & 8 deletions console/test/GraknConsoleIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.common.collect.Sets;
import grakn.core.common.util.GraknVersion;
import grakn.core.console.GraknConsole;
import grakn.core.graql.exception.GraqlQueryException;
import grakn.core.rule.GraknTestServer;
import graql.lang.Graql;
import io.grpc.Status;
Expand Down Expand Up @@ -138,13 +139,14 @@ public void when_writingToDifferentKeyspaces_expect_theyDoNotGetMixedUp() {
runConsoleSessionWithoutExpectingErrors("define bar-bar sub entity;\ncommit\n", "-k", "bar");

String fooFooInFoo = runConsoleSessionWithoutExpectingErrors("match foo-foo sub entity; get; count;\n", "-k", "foo");
String fooFooInBar = runConsoleSessionWithoutExpectingErrors("match foo-foo sub entity; get; count;\n", "-k", "bar");
String barBarInFoo = runConsoleSessionWithoutExpectingErrors("match bar-bar sub entity; get; count;\n", "-k", "foo");
String barBarInBar = runConsoleSessionWithoutExpectingErrors("match bar-bar sub entity; get; count;\n", "-k", "bar");
assertThat(fooFooInFoo, containsString("1"));
assertThat(fooFooInBar, containsString("0"));
assertThat(barBarInFoo, containsString("0"));
assertThat(barBarInBar, containsString("1"));

Response barBarInFoo = runConsoleSession("match bar-bar sub entity; get; count;\n", "-k", "foo");
Response fooFooInBar = runConsoleSession("match foo-foo sub entity; get; count;\n", "-k", "bar");
assertTrue(barBarInFoo.err().contains("label 'bar-bar' not found"));
assertTrue(fooFooInBar.err().contains("label 'foo-foo' not found"));
}

@Test
Expand Down Expand Up @@ -313,11 +315,9 @@ public void when_writingComputeQueries_expect_correctCount() throws Exception {

@Test
public void when_rollback_expect_transactionIsCancelled() {
String[] result = runConsoleSessionWithoutExpectingErrors("define E sub entity;\nrollback\nmatch $x type E; get;\n").split("\n");
Response response = runConsoleSession("define E sub entity;\nrollback\nmatch $x type E; get;\n");

// Make sure there are no results for get query
assertThat(result[result.length - 2], endsWith("> match $x type E; get;"));
assertThat(result[result.length - 1], endsWith("> "));
assertTrue(response.err().contains("label 'E' not found"));
}

@Test
Expand Down
38 changes: 17 additions & 21 deletions server/src/graql/exception/GraqlQueryException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@
import grakn.core.graql.reasoner.atom.Atomic;
import grakn.core.graql.reasoner.query.ReasonerQuery;
import grakn.core.graql.reasoner.query.ResolvableQuery;
import graql.lang.pattern.Pattern;
import graql.lang.statement.Statement;
import graql.lang.statement.Variable;

import java.time.format.DateTimeParseException;
import java.util.Collection;

Expand Down Expand Up @@ -78,6 +78,18 @@ public static GraqlQueryException labelNotFound(Label label) {
return new GraqlQueryException(ErrorMessage.LABEL_NOT_FOUND.getMessage(label));
}

public static GraqlQueryException attributeWithNonAttributeType(Label attributeType) {
return new GraqlQueryException(ErrorMessage.MUST_BE_ATTRIBUTE_TYPE.getMessage(attributeType));
}

public static GraqlQueryException relationWithNonRelationType(Label label) {
return new GraqlQueryException(ErrorMessage.NOT_A_RELATION_TYPE.getMessage(label));
}

public static GraqlQueryException invalidRoleLabel(Label label) {
return new GraqlQueryException(ErrorMessage.NOT_A_ROLE_TYPE.getMessage(label, label));
}

public static GraqlQueryException kCoreOnRelationType(Label label) {
return create("cannot compute coreness of relation type %s.", label.getValue());
}
Expand All @@ -102,14 +114,6 @@ public static GraqlQueryException cannotGetInstancesOfNonType(Label label) {
return GraqlQueryException.create("%s is not a type and so does not have instances", label);
}

public static GraqlQueryException notARelationType(Label label) {
return new GraqlQueryException(ErrorMessage.NOT_A_RELATION_TYPE.getMessage(label));
}

public static GraqlQueryException notARoleType(Label roleId) {
return new GraqlQueryException(ErrorMessage.NOT_A_ROLE_TYPE.getMessage(roleId, roleId));
}

public static GraqlQueryException insertPredicate() {
return new GraqlQueryException(ErrorMessage.INSERT_PREDICATE.getMessage());
}
Expand Down Expand Up @@ -179,14 +183,6 @@ public static GraqlQueryException insertExistingConcept(Statement pattern, Conce
return create("cannot overwrite properties `%s` on concept `%s`", pattern, concept);
}

public static GraqlQueryException noTx() {
return new GraqlQueryException(ErrorMessage.NO_TX.getMessage());
}

public static GraqlQueryException multipleTxs() {
return new GraqlQueryException(ErrorMessage.MULTIPLE_TX.getMessage());
}

public static GraqlQueryException nonPositiveLimit(long limit) {
return new GraqlQueryException(NON_POSITIVE_LIMIT.getMessage(limit));
}
Expand Down Expand Up @@ -231,10 +227,6 @@ public static GraqlQueryException unificationAtomIncompatibility() {
return new GraqlQueryException(ErrorMessage.UNIFICATION_ATOM_INCOMPATIBILITY.getMessage());
}

public static GraqlQueryException nonAtomicQuery(ReasonerQuery query) {
return new GraqlQueryException(ErrorMessage.NON_ATOMIC_QUERY.getMessage(query));
}

public static GraqlQueryException nonGroundNeqPredicate(ReasonerQuery query) {
return new GraqlQueryException(ErrorMessage.NON_GROUND_NEQ_PREDICATE.getMessage(query));
}
Expand Down Expand Up @@ -267,6 +259,10 @@ public static GraqlQueryException unsafeNegationBlock(ResolvableQuery query) {
return new GraqlQueryException(ErrorMessage.UNSAFE_NEGATION_BLOCK.getMessage(query));
}

public static GraqlQueryException usingNegationWithReasoningOff(Pattern pattern) {
return new GraqlQueryException(ErrorMessage.USING_NEGATION_WITH_REASONING_OFF.getMessage(pattern));
}

public static GraqlQueryException disjunctiveNegationBlock() {
return new GraqlQueryException(ErrorMessage.DISJUNCTIVE_NEGATION_BLOCK.getMessage());
}
Expand Down
151 changes: 43 additions & 108 deletions server/src/graql/executor/QueryExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,28 +23,26 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import grakn.benchmark.lib.instrumentation.ServerTracing;
import grakn.core.common.util.CommonUtil;
import grakn.core.concept.Concept;
import grakn.core.concept.Label;
import grakn.core.concept.answer.Answer;
import grakn.core.concept.answer.AnswerGroup;
import grakn.core.concept.answer.ConceptList;
import grakn.core.concept.answer.ConceptMap;
import grakn.core.concept.answer.ConceptSet;
import grakn.core.concept.answer.ConceptSetMeasure;
import grakn.core.concept.answer.Numeric;
import grakn.core.concept.type.SchemaConcept;
import grakn.core.graql.exception.GraqlQueryException;
import grakn.core.graql.executor.property.PropertyExecutor;
import grakn.core.graql.gremlin.GraqlTraversal;
import grakn.core.graql.gremlin.GreedyTraversalPlan;
import grakn.core.graql.reasoner.DisjunctionIterator;
import grakn.core.graql.reasoner.query.ReasonerQueries;
import grakn.core.server.exception.GraknServerException;
import grakn.core.server.session.TransactionOLTP;
import graql.lang.Graql;
import graql.lang.property.HasAttributeProperty;
import graql.lang.property.IsaProperty;
import graql.lang.property.RelationProperty;
import graql.lang.pattern.Conjunction;
import graql.lang.pattern.Disjunction;
import graql.lang.pattern.Pattern;
import graql.lang.property.VarProperty;
import graql.lang.query.GraqlCompute;
import graql.lang.query.GraqlDefine;
Expand All @@ -56,24 +54,23 @@
import graql.lang.query.builder.Filterable;
import graql.lang.statement.Statement;
import graql.lang.statement.Variable;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal;
import org.apache.tinkerpop.gremlin.structure.Edge;
import org.apache.tinkerpop.gremlin.structure.Element;
import org.apache.tinkerpop.gremlin.structure.Vertex;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collector;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal;
import org.apache.tinkerpop.gremlin.structure.Edge;
import org.apache.tinkerpop.gremlin.structure.Element;
import org.apache.tinkerpop.gremlin.structure.Vertex;

import static grakn.core.common.util.CommonUtil.toImmutableList;
import static grakn.core.common.util.CommonUtil.toImmutableSet;
Expand All @@ -97,56 +94,59 @@ public QueryExecutor(TransactionOLTP transaction, boolean infer) {

public Stream<ConceptMap> match(MatchClause matchClause) {


int validatePatternSpanId = ServerTracing.startScopedChildSpan("QueryExecutor.match validate pattern");


//validatePattern
for (Statement statement : matchClause.getPatterns().statements()) {
statement.properties().forEach(property -> validateProperty(property, statement));
}

ServerTracing.closeScopedChildSpan(validatePatternSpanId);


int createStreamSpanId = ServerTracing.startScopedChildSpan("QueryExecutor.match create stream");


Stream<ConceptMap> answerStream;
try {
if (!infer) {
// time to create the traversal plan

int createTraversalSpanId = ServerTracing.startScopedChildSpanWithParentContext("QueryExecutor.match create traversal", createStreamSpanId);
validateClause(matchClause);

GraqlTraversal graqlTraversal = GreedyTraversalPlan.createTraversal(matchClause.getPatterns(), transaction);
if (!infer) {
// time to create the traversal plan

ServerTracing.closeScopedChildSpan(createTraversalSpanId);
int createTraversalSpanId = ServerTracing.startScopedChildSpanWithParentContext("QueryExecutor.match create traversal", createStreamSpanId);

// time to convert plan into a answer stream
int traversalToStreamSpanId = ServerTracing.startScopedChildSpanWithParentContext("QueryExecutor.match traversal to stream", createStreamSpanId);
GraqlTraversal graqlTraversal = GreedyTraversalPlan.createTraversal(matchClause.getPatterns(), transaction);

answerStream = traversal(matchClause.getPatterns().variables(), graqlTraversal);
ServerTracing.closeScopedChildSpan(createTraversalSpanId);

ServerTracing.closeScopedChildSpan(traversalToStreamSpanId);
} else {
// time to convert plan into a answer stream
int traversalToStreamSpanId = ServerTracing.startScopedChildSpanWithParentContext("QueryExecutor.match traversal to stream", createStreamSpanId);

answerStream = traversal(matchClause.getPatterns().variables(), graqlTraversal);

int disjunctionSpanId= ServerTracing.startScopedChildSpanWithParentContext("QueryExecutor.match disjunction iterator", createStreamSpanId);
ServerTracing.closeScopedChildSpan(traversalToStreamSpanId);
} else {

Stream<ConceptMap> stream = new DisjunctionIterator(matchClause, transaction).hasStream();
answerStream = stream.map(result -> result.project(matchClause.getSelectedNames()));
int disjunctionSpanId= ServerTracing.startScopedChildSpanWithParentContext("QueryExecutor.match disjunction iterator", createStreamSpanId);

ServerTracing.closeScopedChildSpan(disjunctionSpanId);
}
} catch (GraqlQueryException e) {
System.err.println(e.getMessage());
answerStream = Stream.empty();
Stream<ConceptMap> stream = new DisjunctionIterator(matchClause, transaction).hasStream();
answerStream = stream.map(result -> result.project(matchClause.getSelectedNames()));

ServerTracing.closeScopedChildSpan(disjunctionSpanId);
}

ServerTracing.closeScopedChildSpan(createStreamSpanId);
return answerStream;
}

//TODO this should go into MatchClause
private void validateClause(MatchClause matchClause){
Disjunction<Conjunction<Pattern>> negationDNF = matchClause.getPatterns().getNegationDNF();
negationDNF.getPatterns().stream()
.flatMap(p -> p.statements().stream())
.map(p -> Graql.and(Collections.singleton(p)))
.forEach(pattern -> ReasonerQueries.createWithoutRoleInference(pattern, transaction).checkValid());
if (!infer) {
boolean containsNegation = negationDNF.getPatterns().stream()
.flatMap(p -> p.getPatterns().stream())
.anyMatch(Pattern::isNegation);
if (containsNegation){
throw GraqlQueryException.usingNegationWithReasoningOff(matchClause.getPatterns());
}
}
}

/**
* @param commonVars set of variables of interest
* @param graqlTraversal graql traversal corresponding to the provided pattern
Expand Down Expand Up @@ -389,69 +389,4 @@ public Stream<ConceptSet> compute(GraqlCompute.Cluster query) {
if (query.getException().isPresent()) throw query.getException().get();
return new ComputeExecutor(transaction).stream(query);
}

private void validateProperty(VarProperty varProperty, Statement statement) {
if (varProperty instanceof IsaProperty) {
validateIsaProperty((IsaProperty) varProperty);
} else if (varProperty instanceof HasAttributeProperty) {
validateHasAttributeProperty((HasAttributeProperty) varProperty);
} else if (varProperty instanceof RelationProperty) {
validateRelationProperty((RelationProperty) varProperty, statement);
}

varProperty.statements()
.map(Statement::getType)
.flatMap(CommonUtil::optionalToStream)
.forEach(type -> {
if (transaction.getSchemaConcept(Label.of(type)) == null) {
throw GraqlQueryException.labelNotFound(Label.of(type));
}
});
}

private void validateIsaProperty(IsaProperty varProperty) {
varProperty.type().getType().ifPresent(type -> {
SchemaConcept theSchemaConcept = transaction.getSchemaConcept(Label.of(type));
if (theSchemaConcept != null && !theSchemaConcept.isType()) {
throw GraqlQueryException.cannotGetInstancesOfNonType(Label.of(type));
}
});
}

private void validateHasAttributeProperty(HasAttributeProperty varProperty) {
Label type = Label.of(varProperty.type());
SchemaConcept schemaConcept = transaction.getSchemaConcept(type);
if (schemaConcept == null) {
throw GraqlQueryException.labelNotFound(type);
}
if (!schemaConcept.isAttributeType()) {
throw GraqlQueryException.mustBeAttributeType(type);
}
}

private void validateRelationProperty(RelationProperty varProperty, Statement statement) {
Set<Label> roleTypes = varProperty.relationPlayers().stream()
.map(RelationProperty.RolePlayer::getRole).flatMap(CommonUtil::optionalToStream)
.map(Statement::getType).flatMap(CommonUtil::optionalToStream)
.map(Label::of).collect(toSet());

Optional<Label> maybeLabel = statement.getProperty(IsaProperty.class)
.map(IsaProperty::type).flatMap(Statement::getType).map(Label::of);

maybeLabel.ifPresent(label -> {
SchemaConcept schemaConcept = transaction.getSchemaConcept(label);

if (schemaConcept == null || !schemaConcept.isRelationType()) {
throw GraqlQueryException.notARelationType(label);
}
});

// Check all role types exist
roleTypes.forEach(roleId -> {
SchemaConcept schemaConcept = transaction.getSchemaConcept(roleId);
if (schemaConcept == null || !schemaConcept.isRole()) {
throw GraqlQueryException.notARoleType(roleId);
}
});
}
}
1 change: 1 addition & 0 deletions server/src/graql/gremlin/GreedyTraversalPlan.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import grakn.core.concept.type.Role;
import grakn.core.concept.type.SchemaConcept;
import grakn.core.concept.type.Type;
import grakn.core.graql.exception.GraqlQueryException;
import grakn.core.graql.gremlin.fragment.Fragment;
import grakn.core.graql.gremlin.fragment.Fragments;
import grakn.core.graql.gremlin.fragment.InIsaFragment;
Expand Down
1 change: 0 additions & 1 deletion server/src/graql/reasoner/DisjunctionIterator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ public DisjunctionIterator(MatchClause matchClause, TransactionOLTP tx) {

private Iterator<ConceptMap> conjunctionIterator(Conjunction<Pattern> conj, TransactionOLTP tx) {
ResolvableQuery query = ReasonerQueries.resolvable(conj, tx).rewrite();
query.checkValid();

boolean doNotResolve = query.getAtoms().isEmpty()
|| (query.isPositive() && !query.isRuleResolvable());
Expand Down
10 changes: 10 additions & 0 deletions server/src/graql/reasoner/atom/binary/AttributeAtom.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import grakn.core.concept.type.SchemaConcept;
import grakn.core.concept.type.Type;
import grakn.core.graql.exception.GraqlQueryException;
import grakn.core.graql.exception.GraqlQueryException;
import grakn.core.graql.reasoner.atom.Atom;
import grakn.core.graql.reasoner.atom.Atomic;
import grakn.core.graql.reasoner.atom.AtomicEquivalence;
Expand Down Expand Up @@ -192,6 +193,15 @@ boolean predicateBindingsEquivalent(Binary at, AtomicEquivalence equiv) {
return predicateBindingsEquivalent(this.getAttributeVariable(), that.getAttributeVariable(), that, equiv);
}

@Override
public void checkValid(){
super.checkValid();
SchemaConcept type = getSchemaConcept();
if (type != null && !type.isAttributeType()) {
throw GraqlQueryException.attributeWithNonAttributeType(type.label());
}
}

@Override
protected Pattern createCombinedPattern(){
Set<Statement> vars = getMultiPredicate().stream()
Expand Down
Loading

0 comments on commit c4b075d

Please sign in to comment.