Skip to content

Commit

Permalink
ISSUE-1027 Support join for having clause (#1039)
Browse files Browse the repository at this point in the history 
* Manager transacton manually

* Add readonly

* some rework

* use getTimeDimension()

* change exception

* ISSUE-1027 Support join for having clause
  • Loading branch information
@hellohanchen @aklish
hellohanchen authored and aklish committed Oct 22, 2019
1 parent 8c0cbc1 commit 5608db4
Show file tree
Hide file tree
Showing 4 changed files with 346 additions and 6 deletions.
60 changes: 60 additions & 0 deletions ...tion/src/main/java/com/yahoo/elide/datastores/aggregation/AggregationDataStoreHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,13 @@
*/
package com.yahoo.elide.datastores.aggregation;

import com.yahoo.elide.core.Path;
import com.yahoo.elide.core.exceptions.InvalidOperationException;
import com.yahoo.elide.core.filter.FilterPredicate;
import com.yahoo.elide.core.filter.expression.AndFilterExpression;
import com.yahoo.elide.core.filter.expression.FilterExpression;
import com.yahoo.elide.core.filter.expression.NotFilterExpression;
import com.yahoo.elide.core.filter.expression.OrFilterExpression;
import com.yahoo.elide.datastores.aggregation.annotation.TimeGrainDefinition;
import com.yahoo.elide.datastores.aggregation.filter.visitor.FilterConstraints;
import com.yahoo.elide.datastores.aggregation.filter.visitor.SplitFilterExpressionVisitor;
Expand Down Expand Up @@ -90,6 +95,10 @@ private void splitFilters() {
FilterConstraints constraints = filterExpression.accept(visitor);
whereFilter = constraints.getWhereExpression();
havingFilter = constraints.getHavingExpression();

if (havingFilter != null) {
validateHavingClause(havingFilter);
}
}

//TODO - Add tests in the next PR.
Expand Down Expand Up @@ -201,4 +210,55 @@ private Set<String> getRelationships() {
return entityProjection.getRelationships().stream()
.map(Relationship::getName).collect(Collectors.toCollection(LinkedHashSet::new));
}

/**
* Validate the having clause before execution. Having clause is not as flexible as where clause,
* the fields in having clause must be either or these two:
* 1. A grouped by dimension in this query
* 2. An aggregated metric in this query
*
* All grouped by dimensions are defined in the entity bean, so the last entity class of a filter path
* must match entity class of the query.
*
* @param havingClause having clause generated from this query
*/
private void validateHavingClause(FilterExpression havingClause) {
if (havingClause instanceof FilterPredicate) {
Path path = ((FilterPredicate) havingClause).getPath();
Path.PathElement last = path.lastElement().get();
Class<?> cls = last.getType();
String field = last.getFieldName();

if (cls != schema.getEntityClass()) {
throw new InvalidOperationException(
String.format(
"Classes don't match when try filtering on %s in having clause of %s.",
cls.getSimpleName(),
schema.getEntityClass().getSimpleName()));
}

if (schema.isMetricField(field)) {
Metric metric = schema.getMetric(field);
if (!metricMap.containsKey(metric)) {
throw new InvalidOperationException(
String.format(
"Metric field %s must be aggregated before filtering in having clause.", field));
}
} else {
if (dimensionProjections.stream().noneMatch(dim -> dim.getName().equals(field))) {
throw new InvalidOperationException(
String.format(
"Dimension field %s must be grouped before filtering in having clause.", field));
}
}
} else if (havingClause instanceof AndFilterExpression) {
validateHavingClause(((AndFilterExpression) havingClause).getLeft());
validateHavingClause(((AndFilterExpression) havingClause).getRight());
} else if (havingClause instanceof OrFilterExpression) {
validateHavingClause(((OrFilterExpression) havingClause).getLeft());
validateHavingClause(((OrFilterExpression) havingClause).getRight());
} else if (havingClause instanceof NotFilterExpression) {
validateHavingClause(((NotFilterExpression) havingClause).getNegated());
}
}
}
21 changes: 15 additions & 6 deletions ...src/main/java/com/yahoo/elide/datastores/aggregation/queryengines/sql/SQLQueryEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,13 +170,16 @@ protected SQLQuery toSQL(Query query, SQLSchema schema) {

if (query.getWhereFilter() != null) {
joinPredicates.addAll(extractPathElements(query.getWhereFilter()));
builder.whereClause("WHERE " + translateFilterExpression(query.getWhereFilter(),
builder.whereClause("WHERE " + translateFilterExpression(
query.getWhereFilter(),
this::generateWhereClauseColumnReference));
}

if (query.getHavingFilter() != null) {
builder.havingClause("HAVING " + translateFilterExpression(query.getHavingFilter(),
(predicate) -> { return generateHavingClauseColumnReference(predicate, query); }));
joinPredicates.addAll(extractPathElements(query.getHavingFilter()));
builder.havingClause("HAVING " + translateFilterExpression(
query.getHavingFilter(),
(predicate) -> generateHavingClauseColumnReference(predicate, query)));
}

if (!query.getDimensions().isEmpty()) {
Expand Down Expand Up @@ -511,14 +514,20 @@ private String generateHavingClauseColumnReference(FilterPredicate predicate, Qu
Path.PathElement last = predicate.getPath().lastElement().get();
Class<?> lastClass = last.getType();

if (! lastClass.equals(query.getSchema().getEntityClass())) {
if (!lastClass.equals(query.getSchema().getEntityClass())) {
throw new InvalidPredicateException("The having clause can only reference fact table aggregations.");
}

Schema schema = schemas.get(lastClass);
Metric metric = schema.getMetric(last.getFieldName());
Class<? extends Aggregation> agg = query.getMetrics().get(metric);
if (metric != null) {
// if the having clause is applied on a metric field, should use aggregation expression
Class<? extends Aggregation> agg = query.getMetrics().get(metric);

return metric.getMetricExpression(agg);
return metric.getMetricExpression(agg);
} else {
// if the having clause is applied on a dimension field, should be the same as a where expression
return generateWhereClauseColumnReference(predicate.getPath());
}
}
}
233 changes: 233 additions & 0 deletions ...test/java/com/yahoo/elide/datastores/aggregation/AggregationDataStoreIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,222 @@ public void havingFilterTest() throws Exception {
runQueryWithExpectedResult(graphQLRequest, expected);
}

/**
* Test the case that a where clause is promoted into having clause.
* @throws Exception
*/
@Test
public void wherePromotionTest() throws Exception {
String graphQLRequest = document(
selection(
field(
"playerStats",
arguments(
argument("filter", "\"overallRating==\\\"Good\\\",lowScore<\\\"45\\\"\"")
),
selections(
field("lowScore"),
field("overallRating"),
field(
"player",
selections(
field("name")
)
)
)
)
)
).toQuery();

String expected = document(
selections(
field(
"playerStats",
selections(
field("lowScore", 35),
field("overallRating", "Good"),
field(
"player",
selections(
field("name", "Jon Doe")
)
)
),
selections(
field("lowScore", 72),
field("overallRating", "Good"),
field(
"player",
selections(
field("name", "Han")
)
)
)
)
)
).toResponse();

runQueryWithExpectedResult(graphQLRequest, expected);
}

/**
* Test the case that a where clause, which requires dimension join, is promoted into having clause.
* @throws Exception
*/
@Test
public void havingClauseJoinTest() throws Exception {
String graphQLRequest = document(
selection(
field(
"playerStats",
arguments(
argument("filter", "\"countryIsoCode==\\\"USA\\\",lowScore<\\\"45\\\"\""),
argument("sort", "\"lowScore\"")
),
selections(
field("lowScore"),
field("countryIsoCode"),
field(
"country",
selections(
field("name"),
field("id")
)
),
field(
"player",
selections(
field("name")
)
)
)
)
)
).toQuery();

String expected = document(
selections(
field(
"playerStats",
selections(
field("lowScore", 35),
field("countryIsoCode", "USA"),
field(
"country",
selections(
field("name", "United States"),
field("id", "840")
)
),
field(
"player",
selections(
field("name", "Jon Doe")
)
)
),
selections(
field("lowScore", 241),
field("countryIsoCode", "USA"),
field(
"country",
selections(
field("name", "United States"),
field("id", "840")
)
),
field(
"player",
selections(
field("name", "Jane Doe")
)
)
)
)
)
).toResponse();

runQueryWithExpectedResult(graphQLRequest, expected);
}

/**
* Test invalid where promotion on a dimension field that is not grouped.
* @throws Exception
*/
@Test
public void ungroupedHavingDimensionTest() throws Exception {
String graphQLRequest = document(
selection(
field(
"playerStats",
arguments(
argument("filter", "\"countryIsoCode==\\\"USA\\\",lowScore<\\\"45\\\"\"")
),
selections(
field("lowScore")
)
)
)
).toQuery();

String errorMessage = "\"Exception while fetching data (/playerStats) : Invalid operation: "
+ "'Dimension field countryIsoCode must be grouped before filtering in having clause.'\"";

runQueryWithExpectedError(graphQLRequest, errorMessage);
}

/**
* Test invalid having clause on a metric field that is not aggregated.
* @throws Exception
*/
@Test
public void nonAggregatedHavingMetricTest() throws Exception {
String graphQLRequest = document(
selection(
field(
"playerStats",
arguments(
argument("filter", "\"highScore<\\\"45\\\"\"")
),
selections(
field("lowScore")
)
)
)
).toQuery();

String errorMessage = "\"Exception while fetching data (/playerStats) : Invalid operation: "
+ "'Metric field highScore must be aggregated before filtering in having clause.'\"";

runQueryWithExpectedError(graphQLRequest, errorMessage);
}

/**
* Test invalid where promotion on a different class than the queried class.
* @throws Exception
*/
@Test
public void invalidHavingClauseClassTest() throws Exception {
String graphQLRequest = document(
selection(
field(
"playerStats",
arguments(
argument("filter", "\"country.isoCode==\\\"USA\\\",lowScore<\\\"45\\\"\"")
),
selections(
field("lowScore")
)
)
)
).toQuery();

String errorMessage = "\"Exception while fetching data (/playerStats) : Invalid operation: "
+ "'Classes don't match when try filtering on Country in having clause of PlayerStats.'\"";

runQueryWithExpectedError(graphQLRequest, errorMessage);
}

@Test
public void whereFilterTest() throws Exception {
String graphQLRequest = document(
Expand Down Expand Up @@ -277,12 +493,29 @@ private void runQueryWithExpectedResult(String graphQLQuery, String expected) th
runQueryWithExpectedResult(graphQLQuery, null, expected);
}

private void runQueryWithExpectedError(
String graphQLQuery,
Map<String, Object> variables,
String errorMessage
) throws IOException {
compareErrorMessage(runQuery(graphQLQuery, variables), errorMessage);
}

private void runQueryWithExpectedError(String graphQLQuery, String errorMessage) throws IOException {
runQueryWithExpectedError(graphQLQuery, null, errorMessage);
}

private void compareJsonObject(ValidatableResponse response, String expected) throws IOException {
JsonNode responseNode = JSON_MAPPER.readTree(response.extract().body().asString());
JsonNode expectedNode = JSON_MAPPER.readTree(expected);
assertEquals(expectedNode, responseNode);
}

private void compareErrorMessage(ValidatableResponse response, String expected) throws IOException {
JsonNode responseNode = JSON_MAPPER.readTree(response.extract().body().asString());
assertEquals(expected, responseNode.get("errors").get(0).get("message").toString());
}

private ValidatableResponse runQuery(String query, Map<String, Object> variables) throws IOException {
return runQuery(toJsonQuery(query, variables));
}
Expand Down
Loading

0 comments on commit 5608db4

Please sign in to comment.