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

Dimension Field Tagging and Dynamic Dimension Field Serilization #137

Merged
merged 6 commits into from
Jan 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 13 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ Current
-------

### Added:

- [Dimension Field Tagging and Dynamic Dimension Field Serilization](https://github.com/yahoo/fili/pull/137)
* Added a new module `fili-navi` for components added to support for Navi
* Added `TaggedDimensionField` and related components in `fili-navi`

- [Added RegisteredLookupDimension and RegisteredLookupExtractionFunction](https://github.com/yahoo/fili/pull/132)
* Added support for druid's RegisteredLookup by adding `RegisteredLookupDimension` and `RegisteredLookupExtractionFunction`
* Added test for `RegisteredLookupDimension` serialization in `RegisteredLookupDimensionToDimensionSpecSpec`
Expand Down Expand Up @@ -44,6 +49,9 @@ Current

### Changed:

- [Dimension Field Tagging and Dynamic Dimension Field Serilization](https://github.com/yahoo/fili/pull/137)
* Changed `fili-core` dimension endpoint `DimensionField` serialization strategy from hard coded static attributes to dynamic serialization based on `jackson` serializer

- [MetricMaker cleanup and simplification](https://github.com/yahoo/fili/pull/127)
* Simplified raw aggregation makers
* `ConstantMaker` now throws an `IllegalArgumentException` wrapping the raw NumberFormatException on a bad argument
Expand Down Expand Up @@ -174,7 +182,11 @@ Current

### Deprecated:

- [Moved to static implementations for numeric and sketch coercion helper methods](https://github.com/yahoo/fili/pull/128)
- [Dimension Field Tagging and Dynamic Dimension Field Serilization](https://github.com/yahoo/fili/pull/137)
* Deprecated `DimensionsServlet::getDimensionFieldListSummaryView` and `DimensionsServlet::getDimensionFieldSummaryView`
since there is no need for it anymore due to the change in serialization of `DimensionField`

- [Moved to static implementations for numeric and sketch coercion helper methods](https://github.com/yahoo/fili/pull/128)
* `MetricMaker.getSketchField(String fieldName)` rather use `MetricMaker.getSketchField(MetricField field)`
* `MetricMaker.getNumericField(String fieldName)` rather use `MetricMaker.getNumericField(MetricField field)`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
package com.yahoo.bard.webservice.data.dimension;

import com.fasterxml.jackson.annotation.JsonFormat;

/**
* Dimension field.
*/
@JsonFormat(shape = JsonFormat.Shape.OBJECT)
public interface DimensionField {

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ public static Map<String, Object> getDimensionFullView(
resultRow.put("name", dimension.getApiName());
resultRow.put("longName", dimension.getLongName());
resultRow.put("description", dimension.getDescription());
resultRow.put("fields", getDimensionFieldListSummaryView(dimension.getDimensionFields()));
resultRow.put("fields", dimension.getDimensionFields());
resultRow.put("values", getDimensionValuesUrl(dimension, uriInfo));
resultRow.put("cardinality", dimension.getCardinality());
resultRow.put(
Expand All @@ -404,7 +404,10 @@ public static Map<String, Object> getDimensionFullView(
* @param dimensionFields Collection of dimension fields to get the summary view for
*
* @return Summary list view of the dimension fields
*
* @deprecated should be private, now the internal usage need is gone, will deprecate in case someone is using it
*/
@Deprecated
public static Set<Map<String, String>> getDimensionFieldListSummaryView(
Collection<DimensionField> dimensionFields
) {
Expand All @@ -419,7 +422,10 @@ public static Set<Map<String, String>> getDimensionFieldListSummaryView(
* @param dimensionField Dimension Field to get the view of
*
* @return Summary view of the dimension field
*
* @deprecated should be private, now the internal usage need is gone, will deprecate in case someone is using it
*/
@Deprecated
public static Map<String, String> getDimensionFieldSummaryView(DimensionField dimensionField) {
Map<String, String> resultRow = new LinkedHashMap<>();
resultRow.put("name", dimensionField.getName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,6 @@ class SerializationResources extends Specification {
}

String getSerializedReponseContext1() {
"""["com.yahoo.bard.webservice.web.responseprocessors.ResponseContext",{"randomHeader":"someHeader","apiMetricColumnNames":["java.util.LinkedHashSet",["metric1, metric2"]],"requestedApiDimensionFields":["java.util.LinkedHashMap",{"ageBracket":["java.util.LinkedHashSet",[["com.yahoo.bard.webservice.data.dimension.BardDimensionField","ID"]]]}]}]"""
"""["com.yahoo.bard.webservice.web.responseprocessors.ResponseContext",{"randomHeader":"someHeader","apiMetricColumnNames":["java.util.LinkedHashSet",["metric1, metric2"]],"requestedApiDimensionFields":["java.util.LinkedHashMap",{"ageBracket":["java.util.LinkedHashSet",[["com.yahoo.bard.webservice.data.dimension.BardDimensionField",{"name":"id","description":"Dimension ID"}]]]}]}]"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should make sure that this don't make anything that was serialized in the old way explode. I'm not 100% clear on what was going on with this before (@archolewa may have some sense of it, since i think it's tied to the async work?), but we want to ensure we're not killing existing serialized things.

Copy link
Collaborator Author

@garyluoex garyluoex Jan 18, 2017

Choose a reason for hiding this comment

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

It seems like it is a custom serialization done in PreResponseSerializationProxy, which is used by at least Digits by MobstorPreResponseStore bind in DigitsBinderFactory. Still not sure if the format of dimension field stored matters or not, can dig deeper or ask Digits people if necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The piece I'm most concerned about is the deserialization of this thing. That's what usually breaks when serialization / deserialization don't line up. Since this capability is lightly (if ever) used over a duration that spans more than a single request, it's probably OK to set this asside.

}
}
44 changes: 44 additions & 0 deletions fili-navi/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<prerequisites>
<maven>3.0</maven>
</prerequisites>

<parent>
<groupId>com.yahoo.fili</groupId>
<artifactId>fili-parent-pom</artifactId>
<version>0.7-SNAPSHOT</version>
</parent>

<artifactId>fili-navi</artifactId>
<packaging>jar</packaging>
<name>Fili: Navi Module</name>
<description>Navi support module for Fili</description>

<licenses>
<license>
<name>The Apache Software License, Version 2.0</name>
<url>http://www.apache.org/licenses/LICENSE-2.0.txt</url>
<distribution>repo</distribution>
</license>
</licenses>

<properties>
<checkstyle.config.location>../checkstyle-style.xml</checkstyle.config.location>
<checkstyle.suppressions.location>../checkstyle-suppressions.xml</checkstyle.suppressions.location>
</properties>

<dependencies>
<dependency>
<groupId>com.yahoo.fili</groupId>
<artifactId>fili-core</artifactId>
</dependency>
<dependency>
<groupId>com.yahoo.fili</groupId>
<artifactId>fili-core</artifactId>
<type>test-jar</type>
</dependency>
</dependencies>
</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright 2017 Yahoo Inc.
// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
package com.yahoo.bard.webservice.data.dimension;

import com.fasterxml.jackson.annotation.JsonValue;

/**
* Interface for tags used for tagging dimension fields to add additional properties implicitly specified by its name.
*/
public interface Tag {

/**
* Gets the String representation of the tag.
*
* @return the String representation of the tag
*/
@JsonValue
String getName();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2017 Yahoo Inc.
// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
package com.yahoo.bard.webservice.data.dimension;

import java.util.Set;

/**
* Interface for Dimension fields that expects unique tags to be attached to it to add expressiveness.
*/
public interface TaggedDimensionField extends DimensionField {

/**
* Get a set of tags associated to the current field.
*
* @return a list of tags
*/
Set<? extends Tag> getTags();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Copyright 2017 Yahoo Inc.
// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
package com.yahoo.bard.webservice.data.dimension.impl;

import com.yahoo.bard.webservice.data.dimension.Tag;
import com.yahoo.bard.webservice.util.EnumUtils;

/**
* Default dimension field tag provided to match fili core concepts.
*/
public enum DefaultDimensionFieldTag implements Tag {
/**
* Dimension field tag to match fili concept of a "key" field.
*/
PRIMARY_KEY
;

private final String tagName;

/**
* Constructor, build name using camel cased enum name.
*/
DefaultDimensionFieldTag() {
this.tagName = EnumUtils.camelCase(name());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CamelCase or just LowerCase? (Does LowerCase feel more like tags?)

Copy link
Contributor

Choose a reason for hiding this comment

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

A quick survery of online tag systems suggests that they are mostly flat cased and searched case insensitively. I agree that all-lower is the right impulse.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do make them sortable, I'd make DimensionTag comparable on getName()

}

@Override
public String getName() {
return tagName;
}
}
7 changes: 7 additions & 0 deletions fili-navi/src/main/resources/moduleConfig.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Copyright 2017 Yahoo Inc.
# Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.

#The name of this module (required)
moduleName = fili-navi

moduleDependencies = fili-core
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2017 Yahoo Inc.
// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
package com.yahoo.bard.systemconfig

/**
* Basic module setup check test.
*/
class NaviModuleSetupSpec extends ModuleSetupSpec {

@Override
String getModuleName() {
return "fili-navi"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package com.yahoo.bard.webservice.data.dimension

import static com.yahoo.bard.webservice.data.dimension.TestTaggedDimensionField.TEST_TWO_TAG
import static com.yahoo.bard.webservice.data.dimension.TestTaggedDimensionField.TEST_ONE_TAG
import static com.yahoo.bard.webservice.data.dimension.TestTaggedDimensionField.TEST_NO_TAG
import static com.yahoo.bard.webservice.data.dimension.impl.DefaultDimensionFieldTag.PRIMARY_KEY

import com.fasterxml.jackson.databind.ObjectMapper

import spock.lang.Specification

/**
* Test tagged dimension field behavior and serialization.
*/
class TaggedDimensionFieldSpec extends Specification {

ObjectMapper objectMapper
Tag mockTag
DimensionField noTagField
TaggedDimensionField oneTagField
TaggedDimensionField twoTagField

def setup() {
objectMapper = new ObjectMapper()

mockTag = Mock(Tag)
mockTag.getName() >> "mockTag"

noTagField = TEST_NO_TAG
oneTagField = TEST_ONE_TAG
twoTagField = TEST_TWO_TAG

noTagField.setTags([] as Set<Tag>)
oneTagField.setTags([PRIMARY_KEY] as Set<Tag>)
twoTagField.setTags([PRIMARY_KEY, mockTag] as Set<Tag>)
}

def "Dimension field interface should behave correctly"() {
expect:
oneTagField.getName() == "testOneTag"
oneTagField.getDescription() == "testOneTag description"
oneTagField.getTags() == [PRIMARY_KEY] as Set
twoTagField.getTags() == [PRIMARY_KEY, mockTag] as Set
}

def "Tagged dimension fields serialize as expected"() {
expect:
objectMapper.writeValueAsString(noTagField) == '{"name":"testNoTag","tags":[],"description":"testNoTag description"}'
objectMapper.writeValueAsString(oneTagField) == '{"name":"testOneTag","tags":["primaryKey"],"description":"testOneTag description"}'
objectMapper.writeValueAsString(twoTagField) == '{"name":"testTwoTag","tags":["primaryKey","mockTag"],"description":"testTwoTag description"}'
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright 2017 Yahoo Inc.
// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
package com.yahoo.bard.webservice.data.dimension;

import com.yahoo.bard.webservice.util.EnumUtils;

import java.util.Collections;
import java.util.Set;

/**
* Test implementation of TaggedDimensionField used in testings.
*/
public enum TestTaggedDimensionField implements TaggedDimensionField {
TEST_NO_TAG,
TEST_ONE_TAG,
TEST_TWO_TAG
;
Copy link
Contributor

Choose a reason for hiding this comment

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

If these fields exist only to illustrate behavior on single tag, multiple tags and no tags, you might want to just rename the dimensions to reflect that purpose.

Also, would it be clearer to instantiate these tags as singletons in the test class? That would make it easer to see what you're testing by putting it next to the test itself, if the test is the only place these are being consumed.


private final String name;
private Set<? extends Tag> tags;

/**
* Constructor.
*/
TestTaggedDimensionField() {
this.name = EnumUtils.camelCase(name());
this.tags = Collections.emptySet();
}

public void setTags(Set<? extends Tag> tags) {
this.tags = tags;
}

@Override
public String getName() {
return name;
}

@Override
public String getDescription() {
return getName() + " description";
}

@Override
public Set<? extends Tag> getTags() {
return tags;
}
}
7 changes: 7 additions & 0 deletions fili-navi/src/test/resources/testApplicationConfig.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Copyright 2017 Yahoo Inc.
# Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.

#The name of this module (required)
moduleName = fili-navi

moduleDependencies = fili-core
1 change: 1 addition & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
<modules>
<module>fili-system-config</module>
<module>fili-core</module>
<module>fili-navi</module>
<module>fili</module>
<module>fili-wikipedia-example</module>
</modules>
Expand Down