Refactor PhysicalTableDefinition and Update BaseTableLoader #207

garyluoex · 2017-03-29T17:06:41Z

PhysicalTableDefinition is now an abstract class, construct using ConcretePhysicalTableDefinition instead
PhysicalTableDefinition now requires a build methods to be implemented that builds a physical table
BaseTableLoader now constructs physical tables by calling build method on PhysicalTableDefinitions in buildPhysicalTablesWithDependency
buildDimensionSpanningTableGroup method in BaseTableLoader now uses loadPhysicalTablesWithDependency instead of removed loadPhysicalTables
buildDimensionSpanningTableGroup method in BaseTableLoader now does not take druid metric as arguments, instead PhysicalTableDefinition does

garyluoex · 2017-04-11T15:23:25Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+            LOG.error("Unable to resolve physical table dependency for physical table: " + currentTableName.asName());
+            throw new RuntimeException("Unable to resolve physical table dependency for physical table: " +
+                    currentTableName
+                    .asName());


Move up a line

Actually, I think this should wrap differently in general:

throw new RuntimeException( "Unable to resolve physical table dependency for physical table: " + currentTableName.asName() );

cdeszaq

Partial feedback.

cdeszaq · 2017-04-11T17:31:52Z

...-core/src/main/java/com/yahoo/bard/webservice/data/config/table/PhysicalTableDefinition.java

-import com.google.common.collect.ImmutableSet;
-
-import org.joda.time.DateTimeZone;
+import avro.shaded.com.google.common.collect.ImmutableSet;


Wrong import

cdeszaq · 2017-04-11T17:59:55Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

@@ -159,6 +155,113 @@ public void loadLogicalTableWithGranularities(
    }

    /**
+     * Load a new physical table into the dictionary and return the loaded physical table.


This JavaDoc doesn't seem to do a good job at explaining the name of the method. In particular, why does the method call out WithDependency? What does that mean? The doc doesn't indicate what's going on regarding that.

cdeszaq · 2017-04-11T18:02:02Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

@@ -159,6 +155,113 @@ public void loadLogicalTableWithGranularities(
    }

    /**
+     * Load a new physical table into the dictionary and return the loaded physical table.
+     *
+     * @param definitions  A config object for the physical table


Should say something like table configurations to load? (ie. plural instead of singular)

cdeszaq · 2017-04-11T18:02:35Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

@@ -159,6 +155,113 @@ public void loadLogicalTableWithGranularities(
    }

    /**
+     * Load a new physical table into the dictionary and return the loaded physical table.


Should be plural instead of singular?

cdeszaq · 2017-04-11T18:02:46Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+     * @param definitions  A config object for the physical table
+     * @param dictionaries  The resource dictionaries for reading and storing resource data
+     *
+     * @return The physical table created


should be plural?

cdeszaq · 2017-04-11T18:15:56Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+        // If the dependent table definition is currently being build or missing
+        if (Objects.isNull(currentTableDefinition)) {
+            LOG.error("Unable to resolve physical table dependency for physical table: " + currentTableName.asName());
+            throw new RuntimeException("Unable to resolve physical table dependency for physical table: " +


Extract the message into a single String.format and pass the same string to both users, rather than duplicate the message building.

cdeszaq · 2017-04-11T20:53:43Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+        // If the dependent table definition is currently being build or missing
+        if (Objects.isNull(currentTableDefinition)) {
+            LOG.error("Unable to resolve physical table dependency for physical table: " + currentTableName.asName());
+            throw new RuntimeException("Unable to resolve physical table dependency for physical table: " +


May also want to indicate that this error could be because of a circular dependency, since we're not sure at this point why the definition isn't available.

cdeszaq · 2017-04-11T20:57:04Z

...-core/src/main/java/com/yahoo/bard/webservice/data/config/table/PhysicalTableDefinition.java

-    ) {
-        this(name, grain.buildZonedTimeGrain(DateTimeZone.UTC), dimensions);
-    }
+    public abstract Optional<PhysicalTable> build(


Why does this return Optional? When do we ever expect that a table couldn't be built that isn't an error condition?

Not much now, maybe good for debugging, but it feels right that it should return an Optional when you build something. Maybe some fili application will extend some new dimension and dimension definition that will fail to build for some reason. Let me know if you want me to remove the Optional or not.

I left a comment elsewhere with deeper thoughts. tldr: Yes, let's remove the Optional wrapper.

cdeszaq · 2017-04-11T21:01:11Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+            physicalTableDictionary.put(currentPhysicalTable.getName(), currentPhysicalTable);
+        } else {
+            LOG.error("Unable to build physical table: " + currentTableDefinition.getName().asName());
+            throw new RuntimeException("Unable to build physical table: " + currentTableDefinition.getName().asName());


Extract the message into a single String.format and pass the same string to both users, rather than duplicate the message building.

cdeszaq · 2017-04-11T22:09:12Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+        Map<TableName, PhysicalTableDefinition> mutableCopyDefinitionMap =
+                buildPhysicalTableDefinitionDictionary(definitions);
+
+        definitions.forEach(definition -> buildPhysicalTablesWithDependency(


This construct feels odd... It's like we're doing a recursion, but modifying a structure and filling up a bucket as we go. I think things would be much cleaner if we used a more traditional DepthFirstBuilding approach, where the return value of the method returns a LinkedHashSet of all of the tables it built, or perhaps just the 1 table it wanted to build.

michael-mclawhorn · 2017-04-12T18:00:11Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+        LinkedHashSet<PhysicalTable> physicalTables = loadPhysicalTablesWithDependency(
+                tableDefinitions,
+                dictionaries
+        );


Per discussion yesterday, for this to work we need to have both the list of table names or table definitions which define the table group and ALSO have the total set of table definitions for building dependent tables.

cdeszaq

Comments for non-test changes

cdeszaq · 2017-04-13T19:45:14Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

     *
     * @return A table group binding all the tables for a logical table view together.
     */
    public TableGroup buildDimensionSpanningTableGroup(
-            Set<ApiMetricName> apiMetrics,
-            Set<FieldName> druidMetrics,
+            Set<TableName> tableNames,


Do we need to include tableNames as a parameter? Can we just get them from the tableDefinitions instead?

tableDefinitions is a super set of tableNames

I see. Though it's not that tableDefinitions is a superset of tableNames if we're building a table group using a table that's already been built into another group. Perhaps we could rename tableNames to something like tablesInTableGroup or something that indicates their purpose more clearly?

cdeszaq · 2017-04-13T19:45:48Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

-            physicalTables.add(table);
-        }
+        Map<String, PhysicalTableDefinition> availableTableDefinitions =
+                buildPhysicalTableDefinitionDictionary(tableDefinitions);


This should wrap on the parameter list, not the assignment.

cdeszaq · 2017-04-13T19:49:09Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+        LinkedHashSet<PhysicalTable> physicalTables = tableNames.stream()
+                .map(
+                        tableName -> physicalTableDictionary.computeIfAbsent(
+                                tableName.asName(),


This call can be pulled up as a primary map step: .map(TableName::asName)

cdeszaq · 2017-04-13T19:50:43Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+                                key -> buildPhysicalTablesWithDependency(key, availableTableDefinitions, dictionaries)
+                        )
+                ).collect(Collectors.toCollection(LinkedHashSet::new));
+

        //Derive the logical dimensions by taking the union of all the physical dimensions


Since we're messing with this method, it would be nice to lift this comment up into the JavaDoc somehow, since this is the core of what the name of the method means, and the existing JavaDoc doesn't really address the "dimension spanning" aspect at all.

Sorry if this repeated elsewhere, I don't think these are 'logical' or 'physical' dimensions, just dimensions here. '..all the dimensions from tables in the group' perhaps.

cdeszaq · 2017-04-13T19:54:00Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

@@ -159,6 +167,138 @@ public void loadLogicalTableWithGranularities(
    }

    /**
+     * Iterate through given definitions and builds all corresponding physical table with satisfied dependency.
+     *
+     * @param currentTableName  Iterator for the mutable definition


How is this an iterator? It looks like it's just a String?

Param still seems to have an odd description?

cdeszaq · 2017-04-13T20:48:44Z

...c/main/java/com/yahoo/bard/webservice/data/config/table/ConcretePhysicalTableDefinition.java

+    public Optional<PhysicalTable> build(
+            ResourceDictionaries dictionaries,
+            DataSourceMetadataService metadataService
+    ) {


I think this can unwrap

cdeszaq · 2017-04-13T21:05:08Z

...-core/src/main/java/com/yahoo/bard/webservice/data/config/table/PhysicalTableDefinition.java

+        this.timeGrain = timeGrain;
+        this.metricNames = ImmutableSet.copyOf(metricNames);
+        this.dimensionConfigs = ImmutableSet.copyOf(dimensionConfigs);
+        this.logicalToPhysicalNames = Collections.unmodifiableMap(buildLogicalToPhysicalNames(dimensionConfigs));


If we really want this class to be primarily a holder for data needed to build a physical table, we need a constructor that takes this logicalToPhysicalNames mapping in as a parameter, rather than just assuming we can build it from the dimension configs.

The reason for this stems from the reason for the mapping in the first place: Dimensions do not have the same physical name on all physical tables. Because of that, you'll either need to have a complete set of DimensionConfig objects for every table, or you need to be able to pass the map into the PTDefinition directly.

(Note that this concern likely ripples down to the constructors of the subclasses as well.

cdeszaq · 2017-04-13T21:06:47Z

...-core/src/main/java/com/yahoo/bard/webservice/data/config/table/PhysicalTableDefinition.java

+    public abstract Set<TableName> getDependentTableNames();
+
+    /**
+     * Given a set of metric names and a data source metadata service, build the corresponding physical table.


I don't think this javadoc is correct?

cdeszaq · 2017-04-13T21:10:54Z

...-core/src/main/java/com/yahoo/bard/webservice/data/config/table/PhysicalTableDefinition.java

-    ) {
-        this(name, grain.buildZonedTimeGrain(DateTimeZone.UTC), dimensions);
-    }
+    public abstract Optional<PhysicalTable> build(


I left a comment elsewhere with deeper thoughts. tldr: Yes, let's remove the Optional wrapper.

cdeszaq · 2017-04-13T21:19:14Z

...-core/src/main/java/com/yahoo/bard/webservice/data/config/table/PhysicalTableDefinition.java

+                                    if (physicalName == null) {
+                                        throw new RuntimeException("No physical name found for dimension: "
+                                                + config.getApiName());
+                                    }


I'm not sure this behavior is correct, but I'm also not sure what the correct behavior is.

Per the docs on DimensionConfig::getPhysicalName, this value: This field (if set) is used as the only physical name, which means that it is OK not to have a value for this field. It also means that if it's not set, the expectation is that there is more than 1 physical name for this dimension, and that information is being set on the PhysicalTables through the mapping directly.

That said, if this method is being made available as a default / helper (ie. if no mapping was provided), which is the expected use of this I think, then there should be a single physical name present for each dimension.

The other concern I have is that by throwing an exception, we're forcing users to specify the physical name of the dimension. While I think it's highly likely that most Fili instances will have different API names and physical names for dimensions, would it ease onboarding (ie. reduce required configuration) if we instead had this method log a warning, and then use the API name as the default physical name?

I would prefer that this not even be a warning. I think we should simply default the physical name to apiName.

Perhaps not a warning, but log something so that it's not surprising that we're filling in the blanks. Info or Debug levels would be fine by me, as long as the method indicates it's going to do this.

cdeszaq

Nothing big for the tests, other than having more descriptive names for the definitions.

cdeszaq · 2017-04-13T21:23:50Z

...-core/src/test/groovy/com/yahoo/bard/webservice/data/config/table/BaseTableLoaderSpec.groovy

+        Optional<PhysicalTable> build(
+                ResourceDictionaries dictionaries,
+                DataSourceMetadataService metadataService
+        ) {


cdeszaq · 2017-04-13T21:27:12Z

...-core/src/test/groovy/com/yahoo/bard/webservice/data/config/table/BaseTableLoaderSpec.groovy

+    PhysicalTableDefinition definition3
+    PhysicalTableDefinition definition4
+    PhysicalTableDefinition definition5
+    PhysicalTableDefinition definition6


Can we give these more descriptive names? Seeing it in code, definition4 means nothing, but selfDependentDefinition is much more meaningful, for example.

cdeszaq · 2017-04-13T21:36:15Z

...-core/src/test/groovy/com/yahoo/bard/webservice/data/config/table/BaseTableLoaderSpec.groovy

        dimNames = TestApiDimensionName.getByLogicalTable(SHAPES)
+        tableNames = physDefs.stream().map({it -> it.getName()}).collect(Collectors.toSet())


Since we're in groovy, it's generally more idiomatic to use groovy's built-in collection processing, closures, and property access:

physDefs.collect {it.name} as Set

cdeszaq · 2017-04-13T21:39:40Z

...-core/src/test/groovy/com/yahoo/bard/webservice/data/config/table/BaseTableLoaderSpec.groovy

-        dicts.physicalDictionary.containsValue(t1)
-        dicts.physicalDictionary.containsValue(t2)
+        dicts.physicalDictionary.size() == 6
+        group.physicalTables.stream().map({it.getTableName()}).collect(Collectors.toSet()) as Set == tableNames


could be groovier: group.physicalTables.collect {it.tableName} as Set

_{Note: Applies elsewhere in PR}

cdeszaq · 2017-04-13T21:42:58Z

...-core/src/test/groovy/com/yahoo/bard/webservice/data/config/table/BaseTableLoaderSpec.groovy

-        List<PhysicalTableDefinition> allDefs = physDefs as List
+    def "loading physical tables with dependency loads all satisfied dependency physical tables"() {
+        given:
+        Set<TableName> currentTableNames = [definition1.getName(), definition2.getName(), definition3.getName()]


property access is groovier: definition1.name, definition2.name, definition3.name

_{Note: Applies elsewhere in PR}

So we are not testing the behavior of getName() anymore?

property access in groovy goes through the getter/setter methods. If you want direct field access, it's a different operator (something like definition1@.name I think). Groovy realized that the more common case is getter/setter access, rather than field access, and so they made accessing JavaBean-style data easier.

michael-mclawhorn

Have some feedback. I need to do a fresh run at this after the next round of changes.

michael-mclawhorn · 2017-04-13T16:52:00Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+                            "circular dependency", currentTableName);
+
+            LOG.error(message);
+            throw new RuntimeException(message);


You could make this an IllegalStateException

michael-mclawhorn · 2017-04-14T19:34:52Z

...-core/src/main/java/com/yahoo/bard/webservice/data/config/table/PhysicalTableDefinition.java

+                                    if (physicalName == null) {
+                                        throw new RuntimeException("No physical name found for dimension: "
+                                                + config.getApiName());
+                                    }


I would prefer that this not even be a warning. I think we should simply default the physical name to apiName.

michael-mclawhorn · 2017-04-14T20:09:14Z

CHANGELOG.md

+    * `BaseTableLoader` now constructs physical tables by calling `build` method on `PhysicalTableDefinition`s in `buildPhysicalTablesWithDependency`
+    * `buildDimensionSpanningTableGroup` method in `BaseTableLoader` now uses `loadPhysicalTablesWithDependency` instead of removed `loadPhysicalTables`
+    * `buildDimensionSpanningTableGroup` method in `BaseTableLoader` now does not take druid metric as arguments, instead `PhysicalTableDefinition` does
+


spelling: druid metrics

michael-mclawhorn · 2017-04-14T20:13:21Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java

+                                key -> buildPhysicalTablesWithDependency(key, availableTableDefinitions, dictionaries)
+                        )
+                ).collect(Collectors.toCollection(LinkedHashSet::new));
+

        //Derive the logical dimensions by taking the union of all the physical dimensions


Sorry if this repeated elsewhere, I don't think these are 'logical' or 'physical' dimensions, just dimensions here. '..all the dimensions from tables in the group' perhaps.

… dependency

garyluoex force-pushed the RefactorPhysicalDef branch 2 times, most recently from cb7774a to ed06b70 Compare March 29, 2017 17:24

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

garyluoex force-pushed the RefactorPhysicalDef branch from ed06b70 to 4a10981 Compare March 29, 2017 21:47

garyluoex added NEED 2 REVIEWS REVIEWABLE and removed WIP labels Mar 29, 2017

garyluoex mentioned this pull request Mar 29, 2017

Add Permissive Concrete Physical Table Definition #212

Merged

garyluoex added the NEED CHANGES label Mar 30, 2017

garyluoex force-pushed the RefactorPhysicalDef branch from 4a10981 to 4e58f37 Compare April 3, 2017 15:53

garyluoex removed the NEED CHANGES label Apr 3, 2017

garyluoex force-pushed the RefactorPhysicalDef branch from 4e58f37 to 9847d6f Compare April 3, 2017 17:13

garyluoex mentioned this pull request Apr 6, 2017

Config slurper master #191

Closed

will-lauer approved these changes Apr 11, 2017

View reviewed changes

garyluoex commented Apr 11, 2017

View reviewed changes

michael-mclawhorn self-requested a review April 11, 2017 20:01

garyluoex added NEED 1 REVIEW and removed NEED 2 REVIEWS labels Apr 11, 2017

cdeszaq self-assigned this Apr 11, 2017

michael-mclawhorn self-assigned this Apr 11, 2017

garyluoex added the NEED CHANGES label Apr 11, 2017

cdeszaq requested changes Apr 11, 2017

View reviewed changes

michael-mclawhorn reviewed Apr 12, 2017

View reviewed changes

garyluoex removed the NEED CHANGES label Apr 12, 2017

cdeszaq mentioned this pull request Apr 13, 2017

Support Filtering Segment Availability from Availability Service #77

Closed

cdeszaq requested changes Apr 13, 2017

View reviewed changes

michael-mclawhorn reviewed Apr 14, 2017

View reviewed changes

garyluoex force-pushed the RefactorPhysicalDef branch from 621a17b to 8f4da9a Compare April 17, 2017 19:10

cdeszaq approved these changes Apr 17, 2017

View reviewed changes

michael-mclawhorn approved these changes Apr 17, 2017

View reviewed changes

garyluoex added 5 commits April 17, 2017 15:28

Refactor PhysicalTableDefinition and Update BaseTableLoader for table…

3464303

… dependency

Modified according to rick's concerns

fa7504c

Modified according to rick and mike's comments

40dc91d

Fix odd param name according to rick's comment

9914386

Final comment touch up

0dfa45f

garyluoex force-pushed the RefactorPhysicalDef branch from 424169d to 0dfa45f Compare April 17, 2017 21:08

garyluoex added MERGEABLE and removed NEED 1 REVIEW REVIEWABLE labels Apr 17, 2017

cdeszaq merged commit 08d0467 into master Apr 17, 2017

garyluoex deleted the RefactorPhysicalDef branch April 17, 2017 21:41

		dimNames = TestApiDimensionName.getByLogicalTable(SHAPES)
		tableNames = physDefs.stream().map({it -> it.getName()}).collect(Collectors.toSet())

Refactor PhysicalTableDefinition and Update BaseTableLoader #207

Refactor PhysicalTableDefinition and Update BaseTableLoader #207

Conversation

garyluoex commented Mar 29, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyluoex Apr 11, 2017 • edited Loading

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

cdeszaq Apr 14, 2017 • edited Loading

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

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

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

garyluoex commented Mar 29, 2017 •

edited

Loading

garyluoex Apr 11, 2017 •

edited

Loading

cdeszaq Apr 14, 2017 •

edited

Loading