Create a default base class for all dimension types #177

asifmansoora · 2017-02-17T20:59:53Z

Create a common base class for Dimension Config , Lookup Dimension Config , Registered Lookup Dimension Config and DimensionName Interface
Made the changes to Wikipedia example to include the new Default Dimension Config class

cdeszaq

Changelog entry needed.

Otherwise, just some fairly small things I think. Thanks for the submission!

cdeszaq · 2017-02-17T21:22:29Z

...re/src/main/java/com/yahoo/bard/webservice/data/config/dimension/DefaultDimensionConfig.java

@@ -0,0 +1,134 @@
+// Copyright 2016 Yahoo Inc.


(May apply to other new files, I won't comment in those places)

Sure. Changing it in all the new files

cdeszaq · 2017-02-17T21:26:11Z

...re/src/main/java/com/yahoo/bard/webservice/data/config/dimension/DefaultDimensionConfig.java

+    return physicalName;
+  }
+
+  public String getDescription() {


All of these methods, if they override a method declared in the interface, should have an @Override annotation.

cdeszaq · 2017-02-17T21:27:13Z

...re/src/main/java/com/yahoo/bard/webservice/data/config/dimension/DefaultDimensionConfig.java

+
+
+/**
+ * A Dimension instance holds all of the information needed to construct a Dimension.


This thing is not a dimension instance, since it's not a Dimension, right?

Yes, you are right, changing the description of Default..classes

cdeszaq · 2017-02-17T21:36:24Z

...re/src/main/java/com/yahoo/bard/webservice/data/config/dimension/DefaultDimensionConfig.java

+     * @param keyValueStore  The key value store holding dimension row data.
+     * @param searchProvider  The search provider for field value lookups on this dimension.
+     */
+    public DefaultDimensionConfig(


Since this includes a KeyValueStore and a SearchProvider, it's actually not the config for a general dimension, but it's the config for a KeyValueStoreDimension (which happens to be Fili's default).

So, perhaps a better name is DefaultKeyValueStoreDimensionConfig?

cdeszaq · 2017-02-17T21:40:25Z

.../main/java/com/yahoo/bard/webservice/data/config/dimension/LookupDefaultDimensionConfig.java

+   * @param keyValueStore  The key value store holding dimension row data.
+   * @param searchProvider  The search provider for field value lookups on this dimension.
+   * @param namespaces  A list of namespaces used to configure the Lookup dimension.
+   */


These javadocs need to call out the differences between these two constructors. It's not obvious what the differences are at a quick glance.

cdeszaq · 2017-02-17T21:44:41Z

.../com/yahoo/bard/webservice/data/config/dimension/RegisteredLookupDefaultDimensionConfig.java

+   * @param defaultDimensionFields  The default set of fields for this dimension to be shown in the response.
+   * @param keyValueStore  The key value store holding dimension row data.
+   * @param searchProvider  The search provider for field value lookups on this dimension.
+   * @param lookups  A list of lookups used to configure the Lookup dimension.


Again, need to document the differences in these constructors

cdeszaq · 2017-02-17T21:44:57Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/names/DimensionName.java

+package com.yahoo.bard.webservice.data.config.names;
+
+/**
+ * Defines the name of a Dimension.


what name? Physical? Logical? etc.

I'm not sure dimensions have a 'physical name', dimension+table combos have a physical name, perhaps.

cdeszaq · 2017-02-17T21:49:14Z

...-example/src/main/java/com/yahoo/wiki/webservice/data/config/names/WikiApiDimensionName.java

@@ -38,4 +49,16 @@
    public String asName() {
        return camelName;
    }
+
+    public String getDescription() {


If this thing is a DimensionName object, why are we stuffing all of these other parameters in there as well? They don't seem to have anything to do with names.

possibly just change the class name to WikiApiDimensionConfigInfo or summary or something.

cdeszaq · 2017-02-17T21:50:23Z

...re/src/main/java/com/yahoo/bard/webservice/data/config/dimension/DefaultDimensionConfig.java

+        @NotNull String physicalName,
+        @NotNull String description,
+        @NotNull String longName,
+        @NotNull String category,


Not all of these are required I don't think. In particular, description, physicalName, longName, and category are optional and have defaults provided when the actual dimensions are made I think.

I'm not necessarily opposed to having the default config objects be stricter or better encouraging of best practices, but I just wanted to point these differences out.

Don't make the four Rick identified be required to be NotNull, please.

cdeszaq · 2017-02-17T21:52:53Z

...-example/src/main/java/com/yahoo/wiki/webservice/data/config/names/WikiApiDimensionName.java

+    CONTINENT("Description of Continent", "wikipedia continent", "General"),
+    COUNTRY("Description of Country", "wikipedia country", "General"),
+    REGION("Description of Region", "wikipedia region", "General"),
+    CITY("Description of City", "wikipedia city", "General");


Since this is an example for others to look at for how to set up a good Fili instance, it would be great to show something more than default-style placeholder text for these descriptions, long names, etc.

e.g. be a little more interesting with the descriptions perhaps?

garyluoex · 2017-02-17T22:09:43Z

...re/src/main/java/com/yahoo/bard/webservice/data/config/dimension/DefaultDimensionConfig.java

+/**
+ * A Dimension instance holds all of the information needed to construct a Dimension.
+ */
+public class DefaultDimensionConfig implements DimensionConfig {


I would prefer this to be an abstract class with the name BaseDimensionConfig since we have a BasePhysicalTable

I think the point of this change is so that an app-specific class isn't needed, so making this abstract defeats that purpose. This work originated from some folks standing up a new Fili instance that had also done a previous one, and noticing that they had to implemented the exact same class for each instance in order to implement the DimensionConfig interface.

Can you call out a specific reason for wanting this abstract? What additions would you expect a concrete version of that abstract one to add?

Ok, I misunderstood what this class is. The name should be changed as rick suggested. Also, I think it is a good idea to have a BaseDimensionConfig abstract class so that we can implement DruidBackedDimensionConfig and so on in the future.

garyluoex · 2017-02-17T22:12:19Z

.../main/java/com/yahoo/bard/webservice/data/config/dimension/LookupDefaultDimensionConfig.java

+/**
+ * A Lookup Dimension instance holds all of the information needed to construct a Lookup Dimension.
+ */
+public class LookupDefaultDimensionConfig extends DefaultDimensionConfig implements LookupDimensionConfig {


I think we should move the Default part of the name to the front like DefaultLookupDimensionConfig.

michael-mclawhorn

I'm not sure happy with the inconsistencies of parameter list capitalizations and formatting in javadoc but I'm not going to fuss too much about that.

Basically this looks fine.

michael-mclawhorn · 2017-02-22T17:17:03Z

...re/src/main/java/com/yahoo/bard/webservice/data/config/dimension/DefaultDimensionConfig.java

+        @NotNull String physicalName,
+        @NotNull String description,
+        @NotNull String longName,
+        @NotNull String category,


Don't make the four Rick identified be required to be NotNull, please.

michael-mclawhorn · 2017-02-22T17:21:19Z

...re/src/main/java/com/yahoo/bard/webservice/data/config/dimension/DefaultDimensionConfig.java

+
+    /**
+     * Construct a DefaultDimensionConfig Instance from Dimension Name , dimension fields and  default dimension fields.
+     *


Most one of these opening sentences on the ...DimensionConfig constructors is slightly wrong and contains the same pre-comma whitespace and inconsistent capitalization

michael-mclawhorn · 2017-02-22T17:29:44Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/names/DimensionName.java

+package com.yahoo.bard.webservice.data.config.names;
+
+/**
+ * Defines the name of a Dimension.


I'm not sure dimensions have a 'physical name', dimension+table combos have a physical name, perhaps.

michael-mclawhorn · 2017-02-22T19:07:24Z

...-example/src/main/java/com/yahoo/wiki/webservice/data/config/names/WikiApiDimensionName.java

@@ -38,4 +49,16 @@
    public String asName() {
        return camelName;
    }
+
+    public String getDescription() {


possibly just change the class name to WikiApiDimensionConfigInfo or summary or something.

michael-mclawhorn · 2017-02-22T19:08:28Z

...-example/src/main/java/com/yahoo/wiki/webservice/data/config/names/WikiApiDimensionName.java

+    CONTINENT("Description of Continent", "wikipedia continent", "General"),
+    COUNTRY("Description of Country", "wikipedia country", "General"),
+    REGION("Description of Region", "wikipedia region", "General"),
+    CITY("Description of City", "wikipedia city", "General");


e.g. be a little more interesting with the descriptions perhaps?

cdeszaq

Getting closer! Just some small things I think.

cdeszaq · 2017-02-24T22:30:49Z

...ia-example/src/main/java/com/yahoo/wiki/webservice/data/config/dimension/WikiDimensions.java

@@ -74,9 +78,10 @@ public WikiDimensions() {
     *
     * @return set of dimension configurations
     */
-    public LinkedHashSet<DimensionConfig> getDimensionConfigurationsByApiName(WikiApiDimensionName... dimensionNames) {
+    public LinkedHashSet<DimensionConfig> getDimensionConfigurationsByApiName(
+        WikiApiDimensionConfigInfo... dimensionNames) {


) { should be on the next line down, aligned with the start of the method declaration line

cdeszaq · 2017-02-24T22:31:41Z

...le/src/main/java/com/yahoo/wiki/webservice/data/config/names/WikiApiDimensionConfigInfo.java

@@ -0,0 +1,64 @@
+// Copyright 2016 Yahoo Inc.


cdeszaq · 2017-02-24T22:35:50Z

...le/src/main/java/com/yahoo/wiki/webservice/data/config/names/WikiApiDimensionConfigInfo.java

+    CONTINENT("Continent refers to one of the seven region", "wiki continent", "General"),
+    COUNTRY("Country is identified as a distinct national entity in political geography", "wiki country", "General"),
+    REGION("Regions are areas that are broadly divided by several distinct characteristics ", "wiki region", "General"),
+    CITY("City is a large and permanent human settlement", "wiki city", "General");


No need to change them, since they are accurate, but generally descriptions of dimensions are likely to connect to the domain of the facts for which they are dimensions (usually). For example, while a city is where humans live, in this context it is the city in which the user making the edit was detected to be located. (ie. it's where the user was when they made the edit).

cdeszaq · 2017-02-24T22:36:09Z

...le/src/main/java/com/yahoo/wiki/webservice/data/config/names/WikiApiDimensionConfigInfo.java

+        return camelName;
+    }
+
+    public String getDescription() {


Should all have @Override annotations

Interface DimensionName has only asName() method to be implemented

asifmansoora · 2017-02-24T23:15:35Z

Addressed all the comments

garyluoex

In general good, just some convention stuff in Fili to fix. Otherwise we are good.

garyluoex · 2017-02-28T19:58:14Z

...ava/com/yahoo/bard/webservice/data/config/dimension/DefaultKeyValueStoreDimensionConfig.java

+import java.util.LinkedHashSet;
+import javax.validation.constraints.NotNull;
+
+


Extra line.

garyluoex · 2017-02-28T20:01:48Z

...ava/com/yahoo/bard/webservice/data/config/dimension/DefaultKeyValueStoreDimensionConfig.java

+   * @param searchProvider  The search provider for field value lookups on this dimension.
+   */
+  public DefaultKeyValueStoreDimensionConfig(
+      DimensionName apiName,


Does it make sense that the other constructor requires some of these arguments to be @NotNull but this constructor does not require? And we should just put @NotNull for all the arguments since thats easier. Apply to other files too.

Re: Making all parameters not null: Not all parameters are required, so no, we should not make them all @NotNull

Re: Making the constructors have the same "nullability" for corresponding parameters: I think that's probably a good idea, yes.

So we should take a look at its super class to determine what arguments needs to be @NotNull.

garyluoex · 2017-02-28T20:10:12Z

.../com/yahoo/bard/webservice/data/config/dimension/DefaultRegisteredLookupDimensionConfig.java

+import java.util.LinkedHashSet;
+import java.util.List;
+
+


Extra line.

garyluoex · 2017-02-28T20:12:33Z

fili-core/src/test/java/com/yahoo/bard/webservice/data/config/names/TestDimensionName.java

+    NAME,
+    AGE,
+    ETHNICITY,
+    GENDER;


Move the semicolon to the next line so that when we add things to the enum list in the future, the last item won't be shown as deleted and then re-added due to moving semicolon to the next item. Apply to other files also.

garyluoex · 2017-02-28T20:18:01Z

...ava/com/yahoo/bard/webservice/data/config/dimension/DefaultKeyValueStoreDimensionConfig.java

+      String description,
+      String longName,
+      String category,
+      LinkedHashSet<DimensionField> defaultDimensionFields,


I think this should be fields instead of defaultDimensionFields, default one is the one can be inferred from fields, not the other way around. Apply to other files too.

This is a good idea. And the parameter comment in the JavaDoc should be updated to reflect that this set of fields will also be used for the default fields (ie. all fields on the Dimension will be the default)

garyluoex · 2017-02-28T20:20:39Z

...ia-example/src/main/java/com/yahoo/wiki/webservice/data/config/dimension/WikiDimensions.java

@@ -74,9 +78,11 @@ public WikiDimensions() {
     *
     * @return set of dimension configurations
     */
-    public LinkedHashSet<DimensionConfig> getDimensionConfigurationsByApiName(WikiApiDimensionName... dimensionNames) {
+    public LinkedHashSet<DimensionConfig> getDimensionConfigurationsByApiName(


Should change the name of the method since it does not take ApiName now.

garyluoex · 2017-02-28T20:24:58Z

...le/src/main/java/com/yahoo/wiki/webservice/data/config/names/WikiApiDimensionConfigInfo.java

+     * @param longName  The Long Name is the external, end-user-facing long  name for the dimension.
+     * @param category  The Category is the external, end-user-facing category for the dimension.
+     */
+    WikiApiDimensionConfigInfo(String description, String longName, String category) {


Consider get rid of the category property from the enum and simply return in the method using default category.

@Override public String getCategory() { return Dimension.DEFAULT_CATEGORY; }

Can we make this change? @asifmansoora

addressed it

cdeszaq

This still needs an entry in the CHANGELOG.md file, describing the changes (mostly additions of default implementations, it looks like).

Once that's in, we can merge this right away.

cdeszaq requested changes Feb 17, 2017

View reviewed changes

garyluoex requested changes Feb 17, 2017

View reviewed changes

michael-mclawhorn requested changes Feb 22, 2017

View reviewed changes

asifmansoora force-pushed the Refactor-Dimension-Config branch from d2cc90e to 99a4ca8 Compare February 24, 2017 21:27

cdeszaq requested changes Feb 24, 2017

View reviewed changes

cdeszaq approved these changes Feb 24, 2017

View reviewed changes

garyluoex requested changes Feb 28, 2017

View reviewed changes

garyluoex added NEED 2 REVIEWS NEED CHANGES REVIEWABLE and removed REVIEWABLE labels Mar 6, 2017

asifmansoora force-pushed the Refactor-Dimension-Config branch from 31ef672 to 711a4ba Compare March 7, 2017 16:39

garyluoex added REVIEWABLE NEED 1 REVIEW NEED CHANGES and removed NEED CHANGES NEED 2 REVIEWS REVIEWABLE labels Mar 7, 2017

garyluoex approved these changes Mar 7, 2017

View reviewed changes

cdeszaq requested changes Mar 7, 2017

View reviewed changes

Create a default base class for all dimension types

d415f82

asifmansoora force-pushed the Refactor-Dimension-Config branch from c5a69e8 to d415f82 Compare March 7, 2017 19:31

cdeszaq merged commit 799ed30 into yahoo:master Mar 7, 2017



		/**
		* A Dimension instance holds all of the information needed to construct a Dimension.

		import java.util.LinkedHashSet;
		import javax.validation.constraints.NotNull;

Create a default base class for all dimension types #177

Create a default base class for all dimension types #177

Conversation

asifmansoora commented Feb 17, 2017

cdeszaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyluoex Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

michael-mclawhorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeszaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asifmansoora commented Feb 24, 2017

garyluoex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeszaq left a comment

Choose a reason for hiding this comment

garyluoex Feb 17, 2017 •

edited

Loading