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.

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

AggregationDataStore: Schema #846

Merged
merged 11 commits into from
Jul 13, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -1214,6 +1214,36 @@ public boolean hasBinding(Class<?> cls) {
return bindJsonApiToEntity.contains(cls);
}

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few general comments (I couldn't find one specific place to put these):

  1. We should rely on the EntityDictionary less throughout. We should build the schema and then use the schema in place of the dictionary where ever possible.
  2. Reflection (checking for presence of annotations) is called repeatedly. Reflection is pretty costly. We should use the schema instead and its related classes (metric & dimension) to answer questions rather than going to the dictionary again and again.
  3. Metric expansion has to happen at query time. We cannot know in advance what agg function to apply during the expansion.
  4. Utils classes (IMO) are usually just misplaced functions. I have some comments on this below, but let's find a proper home for those functions and retire the Utils class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll refactor according to these

* Returns whether or not a specified annotation is present on an entity field or its corresponding method.
*
* @param fieldName The entity field
* @param annotationClass The provided annotation class
*
* @param <A> The type of the {@code annotationClass}
*
* @return {@code true} if the field is annotated by the {@code annotationClass}
*/
public <A extends Annotation> boolean attributeOrRelationAnnotationExists(
Class<?> cls,
String fieldName,
Class<A> annotationClass
) {
return getAttributeOrRelationAnnotation(cls, annotationClass, fieldName) != null;
}

/**
* Returns whether or not a specified field exists in an entity.
*
* @param cls The entity
* @param fieldName The provided field to check
*
* @return {@code true} if the field exists in the entity
*/
public boolean isValidField(Class<?> cls, String fieldName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add new test methods to EntityDictionaryTest for these new functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

return getAllFields(cls).contains(fieldName);
}

/**
* Binds the entity class if not yet bound.
* @param entityClass the class to bind.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.Id;
import javax.persistence.OneToOne;
import javax.persistence.Transient;

public class EntityDictionaryTest extends EntityDictionary {
Expand Down Expand Up @@ -475,4 +476,16 @@ public void testBadLookupEntityClass() {
Assert.assertThrows(IllegalArgumentException.class, () -> lookupEntityClass(null));
Assert.assertThrows(IllegalArgumentException.class, () -> lookupEntityClass(Object.class));
}

@Test
public void testAttributeOrRelationAnnotationExists() {
Assert.assertTrue(attributeOrRelationAnnotationExists(Job.class, "jobId", Id.class));
Assert.assertFalse(attributeOrRelationAnnotationExists(Job.class, "title", OneToOne.class));
}

@Test
public void testIsValidField() {
Assert.assertTrue(isValidField(Job.class, "title"));
Assert.assertFalse(isValidField(Job.class, "foo"));
}
}
26 changes: 26 additions & 0 deletions elide-datastore/elide-datastore-aggregation/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,36 @@
<groupId>com.yahoo.elide</groupId>
<artifactId>elide-core</artifactId>
</dependency>

<!-- JPA -->
<dependency>
<groupId>org.hibernate.javax.persistence</groupId>
<artifactId>hibernate-jpa-2.1-api</artifactId>
<version>1.0.0.Final</version>
</dependency>

<!-- Test -->
<dependency>
<groupId>org.testng</groupId>
<artifactId>testng</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>build-helper-maven-plugin</artifactId>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright 2019, Yahoo Inc.
* Licensed under the Apache License, Version 2.0
* See LICENSE file in project root for terms.
*/
package com.yahoo.elide.datastores.aggregation;

import com.yahoo.elide.datastores.aggregation.annotation.Meta;
import com.yahoo.elide.datastores.aggregation.dimension.Dimension;
import com.yahoo.elide.datastores.aggregation.metric.Metric;

import lombok.Getter;

import java.util.Objects;

/**
* Base class that offers common components between {@link Metric} and {@link Dimension}.
*/
public abstract class Column {

@Getter
protected final String name;
@Getter
protected final String longName;
@Getter
protected final String description;
@Getter
protected final Class<?> dataType;

/**
* Constructor.
*
* @param field The entity field or relation that this {@link Column} represents
* @param annotation Provides static meta data about this {@link Column}
* @param fieldType The Java type for this entity field or relation
*
* @throws NullPointerException if {@code field} or {@code fieldType} is {@code null}
*/
public Column(String field, Meta annotation, Class<?> fieldType) {
this.name = Objects.requireNonNull(field, "field");
this.longName = annotation == null || annotation.longName().isEmpty() ? field : annotation.longName();
this.description = annotation == null || annotation.description().isEmpty() ? field : annotation.description();
this.dataType = Objects.requireNonNull(fieldType, "fieldType");
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}
if (other == null || getClass() != other.getClass()) {
return false;
}
final Column column = (Column) other;

return getName().equals(column.getName())
&& getLongName().equals(column.getLongName())
&& getDescription().equals(column.getDescription())
&& getDataType().equals(column.getDataType());
}

@Override
public int hashCode() {
return Objects.hash(getName(), getLongName(), getDescription(), getDataType());
}
}
Loading