Partition table config #259

michael-mclawhorn · 2017-04-27T20:10:18Z

Added dimension value based partition feature.

Configuration of dimension value partitions is done by supplying a map of table names to maps of dimension names to maps of dimension values.
The entry for a given table name describes a set of filters, by dimension. If each dimension successfully applies when combined with the restrictions on a table, then the partition is considered to pertain to the query. A partition which pertains is used for restricting availability.

cdeszaq

Needs changelog

garyluoex

Small things.

garyluoex · 2017-04-28T15:26:01Z

.../java/com/yahoo/bard/webservice/data/config/table/DimensionListPartitionTableDefinition.java

+            ZonedTimeGrain timeGrain,
+            Set<FieldName> metricNames,
+            Set<? extends DimensionConfig> dimensionConfigs,
+            Map<TableName, Map<String, Set<String>>> tablePartDefinitions


We do have a DimensionName now =)

I'll come back to this.

garyluoex · 2017-04-28T15:44:15Z

fili-core/src/main/java/com/yahoo/bard/webservice/table/PhysicalTableSchema.java

@@ -35,7 +37,8 @@ public PhysicalTableSchema(
        super(timeGrain, columns);
        this.timeGrain = timeGrain;

-        this.logicalToPhysicalColumnNames = Collections.unmodifiableMap(logicalToPhysicalColumnNames);
+        this.logicalToPhysicalColumnNames = Collections.unmodifiableMap(new LinkedHashMap<>
+                (logicalToPhysicalColumnNames));


Can we wrap this better?

garyluoex · 2017-04-28T15:49:48Z

...core/src/main/java/com/yahoo/bard/webservice/table/availability/MetricUnionAvailability.java

+            return Objects.equals(metricNames, that.metricNames)
+                    && Objects.equals(availabilitiesToMetricNames, that.availabilitiesToMetricNames);
+        }
+        return super.equals(obj);


Why are we using super's equal if two objects are not the same?

garyluoex · 2017-04-28T16:20:07Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/util/PaginationParameters.java

+    public static final PaginationParameters ONE_RESULT = new PaginationParameters(
+            1,
+            1
+    );


Can unwrap?

garyluoex · 2017-04-28T16:30:15Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/table/resolver/DimensionIdFilterSpec.groovy

+        searchProvider.hasAnyRows(_) >> searchRowsExist
+
+        expect: "filter matches when there are no constraints or they exist and return rows from search provider"
+        expected == filter.emptyConstraintOrAnyRows(dimension, constraintMap)


Expected should be on the right hand side. IDE will display result on the left hand side as actual and right hand side as expected I think.

garyluoex · 2017-04-28T16:35:06Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/table/resolver/DimensionIdFilterSpec.groovy

+        }
+
+        expect: "True as long as the dimension that doesnt return rows is not constrained"
+        dimensionIdFilter.apply(constraint) == success


should probably rename success to something like, expected or participate

cdeszaq

Nothing too big

cdeszaq · 2017-04-28T16:27:09Z

.../java/com/yahoo/bard/webservice/data/config/table/DimensionListPartitionTableDefinition.java

+     * @param timeGrain  Zoned time grain of the table
+     * @param metricNames  The Set of metric names on the table
+     * @param dimensionConfigs  Set of dimensions on the table as dimension configs
+     * @param tablePartDefinitions A map from table names to the dimension value matches for that table


Could we expand a bit here on what the components of the Map are? Map<String, Set<String>> doesn't tell us what the key and value should be.

cdeszaq · 2017-04-28T16:33:54Z

.../main/java/com/yahoo/bard/webservice/data/volatility/DefaultingVolatileIntervalsService.java

-     */
-    public DefaultingVolatileIntervalsService(
-                DefaultingDictionary<PhysicalTable, VolatileIntervalsFunction> intervalsFunctions
-    ) {


Why are we removing this constructor outright instead of deprecating it? (call out in changelog)

cdeszaq · 2017-04-28T16:34:13Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/dimension/SearchProvider.java

+     *
+     * @return true if at least one row is returned.
+     */
+    default boolean hasAnyRows(Set<ApiFilter> filters) {


Call out addition in changelog.

cdeszaq · 2017-04-28T16:37:02Z

fili-core/src/main/java/com/yahoo/bard/webservice/table/BasePhysicalTable.java

@@ -139,4 +140,31 @@ public String getPhysicalColumnName(String logicalName) {
    protected void setAvailability(Availability availability) {
        this.availability = availability;
    }
+
+    @Override
+    public boolean equals(final Object obj) {


final not needed

cdeszaq · 2017-04-28T16:37:21Z

fili-core/src/main/java/com/yahoo/bard/webservice/table/BasePhysicalTable.java

+
+    @Override
+    public boolean equals(final Object obj) {
+        if (obj instanceof BasePhysicalTable) {


Should check instance equality 1st?

cdeszaq · 2017-04-28T16:54:24Z

...com/yahoo/bard/webservice/data/config/table/DimensionListPartitionTableDefinitionSpec.groovy

+                TableName.of("partition"),
+                DAY_UTC,
+                Collections.emptySet(),
+                Collections.emptySet(),


[] as Set

cdeszaq · 2017-04-28T16:54:33Z

...com/yahoo/bard/webservice/data/config/table/DimensionListPartitionTableDefinitionSpec.groovy

+        PartitionCompositeTable expected = new PartitionCompositeTable(
+                TableName.of("partition"),
+                DAY_UTC,
+                (Set<Column>) Collections.emptySet(),


[] as Set

cdeszaq · 2017-04-28T16:57:42Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/data/dimension/DimensionRowSpec.groovy

+        keyField.name >> keyFieldFieldName
+        nonKeyField.name >> nonKeyFieldName
+    }
+    def "healthy initialization returns correct state"() {


blank line between methods

cdeszaq · 2017-04-28T16:57:51Z

...oovy/com/yahoo/bard/webservice/data/volatility/DefaultingVolatileIntervalsServiceSpec.groovy

 import com.yahoo.bard.webservice.util.SimplifiedIntervalList

 import org.joda.time.DateTime
 import org.joda.time.Interval

 import spock.lang.Specification
-


cdeszaq · 2017-04-28T16:58:28Z

...oovy/com/yahoo/bard/webservice/data/volatility/DefaultingVolatileIntervalsServiceSpec.groovy

@@ -59,7 +63,7 @@ class DefaultingVolatileIntervalsServiceSpec extends Specification {
        allGrainQuery.intervals >> fullRange
        allGrainQuery.granularity >> AllGranularity.INSTANCE

-        intervalsFunctions = new DefaultingDictionary<>(defaultFunction)
+        intervalsFunctions = new HashMap<>()
        intervalsFunctions.put(table2, mockFunction2)
        intervalsFunctions.put(table3, mockFunction3)


Just use map literals, now that this is a simple Map?

Patched bug on volatile equals.

Clean up to Strings and Equals. CHANGELOG

QubitPi · 2017-05-05T18:25:49Z

.../java/com/yahoo/bard/webservice/data/config/table/DimensionListPartitionTableDefinition.java

+     * @param timeGrain  Zoned time grain of the table
+     * @param metricNames  The Set of metric names on the table
+     * @param dimensionConfigs  Set of dimensions on the table as dimension configs
+     * @param tablePartDefinitions A map from table names to a map of dimension names to sets of values for those


2 spaces between name and description

QubitPi · 2017-05-05T18:26:42Z

.../java/com/yahoo/bard/webservice/data/config/table/DimensionListPartitionTableDefinition.java

+
+    @Override
+    public Set<TableName> getDependentTableNames() {
+        return tablePartDefinitions.keySet();


I suggest putting tablePartDefinitions.keySet(); to a variable so we don't have to call .keySet() multiple times.

Is keySet expensive? I'm not sure it makes sense to keep a copy of the keySet and the original collection. Also, I think the builder might modify the tablePartDefinitions class during construction and having a copy of the keyset might result in inconsistency.

QubitPi · 2017-05-05T18:27:44Z

.../java/com/yahoo/bard/webservice/data/config/table/DimensionListPartitionTableDefinition.java

+                        entry -> dictionaries.getPhysicalDictionary().get(entry.getKey().asName()),
+                        entry -> new DimensionIdFilter(toDimensionValuesMap(
+                                entry.getValue(),
+                                dictionaries.getDimensionDictionary()


Will it be better to assign dictionaries.getDimensionDictionary() and dictionaries.getPhysicalDictionary() to variables?

QubitPi · 2017-05-05T18:28:06Z

.../java/com/yahoo/bard/webservice/data/config/table/DimensionListPartitionTableDefinition.java

+     * @param dimensionNameMap  The configuration map from dimension names to sets of dimension key values.
+     * @param dimensionDictionary  The dictionary of dimensions to use for binding.
+     *
+     * @return A map of dimensions to dimension key values.


@cdeszaq Lower case "A"?

QubitPi · 2017-05-05T18:30:08Z

fili-core/src/main/java/com/yahoo/bard/webservice/table/BasePhysicalTable.java

+                    && Objects.equals(availability, that.availability);
+        } else {
+            return false;
+        }


else { return false; }

can be simplified to

return false;

QubitPi · 2017-05-05T18:34:56Z

fili-core/src/main/java/com/yahoo/bard/webservice/table/resolver/DimensionIdFilter.java

+        dimensionKeySelectFilters = dimensionMappingValues.entrySet().stream()
+                .map(entry -> new AbstractMap.SimpleEntry<>(
+                        entry.getKey(),
+                        new ApiFilter(entry.getKey(), entry.getKey().getKey(), FilterOperation.in, entry.getValue())


Since we are getting key multiple times, could we assign entry.getKey() to a variable?

It's likely not worth it here, since this is a fairly simple constructor, and the call is likely very cheap. The added complexity isn't worth it, imho.

QubitPi · 2017-05-05T18:35:37Z

fili-core/src/main/java/com/yahoo/bard/webservice/table/resolver/DimensionIdFilter.java

+     * @param dimension  The dimension whose rows are being tested on.
+     * @param constraintFilters  The api filters from the constraint
+     *
+     * @return  True if for this dimension there are rows matching the query filters AND the embedded filters.


Once space between "return" and "True". "true" should be lowercased

QubitPi · 2017-05-05T18:36:09Z

fili-core/src/main/java/com/yahoo/bard/webservice/table/resolver/DimensionIdFilter.java

+
+    @Override
+    public boolean equals(Object obj) {
+        if (obj instanceof DimensionIdFilter) {


should we add this == obj check?

QubitPi · 2017-05-05T18:36:20Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/StreamUtils.java

+     * @param value  The value being appended to the set.
+     * @param <T>  The type of the set
+     *
+     * @return The new set containing the additional value


Lowercase "The"

QubitPi · 2017-05-05T18:38:35Z

...com/yahoo/bard/webservice/data/config/table/DimensionListPartitionTableDefinitionSpec.groovy

+
+    TableName dataSource1 = TableName.of("part1_ds1")
+    TableName dataSource2 = TableName.of("part1_ds2")
+    TableName dataSource3 = TableName.of("part2_ds1")


Is part2Ds1 better for style?

Since these aren't java identifiers I think readability is the biggest concern and this way is clearly more readable.

Equals optimizations

michael-mclawhorn force-pushed the PartitionTableConfig branch from 6cd1154 to 3ef487e Compare April 28, 2017 00:09

cdeszaq reviewed Apr 28, 2017

View reviewed changes

garyluoex requested changes Apr 28, 2017

View reviewed changes

cdeszaq requested changes Apr 28, 2017

View reviewed changes

michael-mclawhorn added 8 commits May 3, 2017 13:54

Refactored out BaseCompositeAvailability

f5f56d0

Added equals capabilities.

0ff0c47

Patched bug on volatile equals.

Tested new DimensionRow method.

7548ceb

Added PartitionDefinition and tests

c73ac5d

Enriched toString

59e438f

Normalizing immutable maps.

ee7a10c

Codacy feedback.

e8ac5e2

PR feedback.

5e655d2

Clean up to Strings and Equals. CHANGELOG

michael-mclawhorn force-pushed the PartitionTableConfig branch from ba062f9 to 5e655d2 Compare May 5, 2017 14:36

michael-mclawhorn added 2 commits May 5, 2017 09:37

Missed one

97dd44c

Fixed style errors.

3910bcc

garyluoex approved these changes May 5, 2017

View reviewed changes

QubitPi suggested changes May 5, 2017

View reviewed changes

QubitPi added NEED 1 REVIEW NEED CHANGES labels May 5, 2017

PR javadoc issues

ee27ce2

Equals optimizations

QubitPi approved these changes May 10, 2017

View reviewed changes

cdeszaq approved these changes May 10, 2017

View reviewed changes

cdeszaq merged commit 872379b into master May 10, 2017

cdeszaq deleted the PartitionTableConfig branch May 10, 2017 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partition table config #259

Partition table config #259

michael-mclawhorn commented Apr 27, 2017

cdeszaq left a comment

garyluoex left a comment

garyluoex Apr 28, 2017

michael-mclawhorn May 3, 2017

garyluoex Apr 28, 2017

garyluoex Apr 28, 2017

garyluoex Apr 28, 2017

garyluoex Apr 28, 2017 •

edited

garyluoex Apr 28, 2017

cdeszaq left a comment

cdeszaq Apr 28, 2017

cdeszaq Apr 28, 2017

cdeszaq Apr 28, 2017

cdeszaq Apr 28, 2017

cdeszaq Apr 28, 2017

cdeszaq Apr 28, 2017

cdeszaq Apr 28, 2017

cdeszaq Apr 28, 2017

cdeszaq Apr 28, 2017

cdeszaq Apr 28, 2017

QubitPi May 5, 2017

QubitPi May 5, 2017

michael-mclawhorn May 5, 2017

QubitPi May 5, 2017

QubitPi May 5, 2017

QubitPi May 5, 2017

QubitPi May 5, 2017

cdeszaq May 9, 2017

QubitPi May 5, 2017

QubitPi May 5, 2017

QubitPi May 5, 2017

QubitPi May 5, 2017

garyluoex May 5, 2017

michael-mclawhorn May 5, 2017

Partition table config #259

Partition table config #259

Conversation

michael-mclawhorn commented Apr 27, 2017

cdeszaq left a comment

Choose a reason for hiding this comment

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

garyluoex Apr 28, 2017 • edited

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

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 Apr 28, 2017 •

edited