Skip to content
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.

Sign up for GitHub

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

ISSUE-1027 Support join for having clause #1039

Merged
merged 10 commits into from
Oct 22, 2019
Merged

ISSUE-1027 Support join for having clause #1039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a86a6e3
Manager transacton manually
Oct 17, 2019
cd05415
Add readonly
Oct 17, 2019
5a3195d
Merge branch 'elide-5.x' of https://github.com/yahoo/elide into elide…
Oct 18, 2019
b3d3e6c
some rework
Oct 18, 2019
ad76293
use getTimeDimension()
Oct 18, 2019
6b0a9ab
change exception
Oct 18, 2019
cf1da84
Merge branch 'elide-5.x' of https://github.com/yahoo/elide into elide…
Oct 21, 2019
efff12e
ISSUE-1027 Support join for having clause
Oct 21, 2019
5bf757e
merge master
Oct 22, 2019
fc9a558
Merge branch 'elide-5.0' into issue-1027
Oct 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions ...tion/src/main/java/com/yahoo/elide/datastores/aggregation/AggregationDataStoreHelper.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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\\\"\"")
hellohanchen marked this conversation as resolved.
Show resolved Hide resolved
),
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