Implement MetricUnionAvailability & MetricUnionCompositeTable #193

QubitPi · 2017-03-10T17:48:45Z

TODO

Add tests to MetricUnionCompositeTable once @cdeszaq @michael-mclawhorn @garyluoex are OK with its implementation.
Update ChangeLog

garyluoex

Modify tests accordingly.

garyluoex · 2017-03-10T22:13:42Z

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

+                )
+                .entrySet()
+                .stream()
+                .anyMatch(entry -> entry.getValue() > 1)) {


Indention is a bit weird, otherwise good.

garyluoex · 2017-03-10T22:17:25Z

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

+     * @param columns The columns associated with all tables
+     */
+    public MetricUnionAvailability(
+            @NotNull Map<Table, Set<String>> metricAvailability,


Change Table to PhysicalTable

garyluoex · 2017-03-10T22:17:51Z

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

+
+        this.columns = columns;
+        this.dataSourceNames = metricAvailability.keySet().stream()
+                .map(table -> TableName.of(table.getName()))


Use getTableName method on PhysicalTable

garyluoex · 2017-03-10T22:19:00Z

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

+        this.metricAvailability = metricAvailability.entrySet().stream()
+                .collect(
+                        Collectors.toMap(
+                                entry -> PhysicalTable.class.cast(entry.getKey()).getAvailability(),


Should not need to cast

Also, this is a strange way to cast. Generally, casts in Java are done like:

entry -> (PhysicalTable) entry.getKey()

garyluoex · 2017-03-10T22:31:33Z

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

+
+    @Override
+    public Map<Column, Set<Interval>> getAllAvailableIntervals() {
+        return metricAvailability.entrySet().stream()


I think there might and will be repeated columns, we need to union those availabilities together I think using group by.

garyluoex · 2017-03-10T22:48:25Z

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

+                intersectionBetween(getRequestDimensions(), other),
+                intersectionBetween(getFilterDimensions(), other),
+                intersectionBetween(getMetricDimensions(), other),
+                metricNames.stream()


Just need the metricNames to intersect other not the dimensions here, change the name to include metricIntersection, leave the dimensions the same.

garyluoex · 2017-03-10T22:55:41Z

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

+                                entry -> constraint.withIntersectedColumnNames(entry.getValue())
+                        )
+                ).entrySet().stream()
+                        .filter(entry -> !entry.getValue().getAllColumnNames().isEmpty())


check if the metric is empty, not including dimensions.

garyluoex · 2017-03-10T22:57:19Z

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

+     *
+     * @return The intersection, containing <tt>Dimension</tt> objects, between the two sets
+     */
+    private static Set<Dimension> intersectionBetween(Set<Dimension> first, Set<String> second) {


Can remove, no need.

garyluoex · 2017-03-10T22:57:49Z

.../test/groovy/com/yahoo/bard/webservice/table/availability/MetricUnionAvailabilitySpec.groovy

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


QubitPi · 2017-03-14T15:08:27Z

The refactor breaks down methods into smaller helper methods so they are more readable and can be tested better.

Tests for getAvailableIntervals will come in the next commit(Although its helper methods are all tested). I don't want the whole PR to be blocked by a single test.

QubitPi · 2017-03-14T21:15:20Z

.../test/groovy/com/yahoo/bard/webservice/table/availability/MetricUnionAvailabilitySpec.groovy

+
+    class TestMetricUnionAvailability extends MetricUnionAvailability {
+
+        List<TestAvailability> testAvailabilities


Does this need to be List<Availability> instead?

Can you briefly explain why do we need a TestAvailability?

Sure, it's explained in the next commit.

I mean... explain why do we need TestAvailability to exist? Explain briefly in this comment.

Since Availability is an interface. An implementation of it, TestAvailability, can return specified value from getAvailableIntervals. We need Availability.getAvailableIntervals to return something in test

I could've mocked Availability and then Availability.getAvailableIntervals >> ..., but that's not allowed in TestMetricUnionAvailability

You should be able to test MetricUnionAvailability directly using mocks for ConcretePhysicalTable or ConcreteAvailability, without having to create another TestMetricUnionAvailability. I will review and figure out how to do this later today, in the mean time try if this is possible

michael-mclawhorn · 2017-03-16T15:56:27Z

CHANGELOG.md

@@ -9,6 +9,10 @@ Current
 -------

 ### Added:
+- [MetricUnionAvailability](https://github.com/yahoo/fili/pull/193)
+    * Added `MetricUnionAvailability` which unions multiple different physical tables together into a single table
+    that contains all columns from all tables so that we can query different columns from different tables at the same time.


This is incorrect.
MetricUnionAvailability joins physical tables with the same dimension schema but non-intersecting metric schemas. Metrics on the MUA are sourced from exactly one of the participating tables.

michael-mclawhorn · 2017-03-16T15:56:48Z

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

+ * An availability which unions multiple different physical tables together into a single table
+ * that contains all columns from all tables so that we can query different columns
+ * from different tables at the same time.
+ */


Wrong as per the CHANGELOG comment above.

michael-mclawhorn · 2017-03-16T16:11:18Z

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

+     * @return true if the map contains duplicate values or false otherwise
+     */
+    private static boolean hasDuplicateValue(Map<PhysicalTable, Set<String>> map) {
+        return map.values().stream()


Metric Column not string, I think

michael-mclawhorn · 2017-03-16T16:14:58Z

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

+        // validate metric uniqueness such that
+        // each table's underlying datasource schema do not have repeated metric column
+        if (hasDuplicateValue(metricAvailability)) {
+            throw new IllegalArgumentException("Metric columns are not unique.");


It might be nice to actually let them know which metric columns are creating the error. Perhaps collect the duplicated metric names and then format them into the error string.

michael-mclawhorn · 2017-03-16T16:20:08Z

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

+     * @return the new <tt>DataSourceConstraint</tt> instance with a new subset of metric names
+     */
+    public DataSourceConstraint withMetricIntersection(Set<String> other) {
+        return new DataSourceConstraint(


'other' here is a set of metric names. Please change the variable name.

michael-mclawhorn · 2017-03-16T16:22:57Z

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

+            Set<Dimension> filterDimensions,
+            Set<Dimension> metricDimensions,
+            Set<String> metricNames,
+            Map<Dimension, Set<ApiFilter>> apiFilters


Should DSC initialize on String metric names or FieldNames?
We need to discuss this.

cdeszaq

Round 1 of comments. Didn't get to the tests.

cdeszaq · 2017-03-23T20:32:36Z

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

+            );
+        }
+
+        dataSourceNames = physicalTables.stream()


I think this needs to aggregate from availabilities not physical tables. In particular, if this is a union over another union (for example), it should collect all of the leaf names. Please make sure there's a test for this case as well.

Good catch.
This is collecting physical table dictionary names and not datasource names.
We need to call 'getDataSourceNames' on the the underlying availabilities.

cdeszaq · 2017-03-23T20:34:24Z

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

+    /**
+     * Constructor.
+     *
+     * @param physicalTables  A set of <tt>PhysicalTable</tt>'s


We should be able to give more information about what this set means. The fact that it's a set of physical tables is indicated already by the type of the parameter, so the rest of the comment should describe the semantics of the parameter (ie. the meaning)

cdeszaq · 2017-03-23T20:35:03Z

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

+     * Constructor.
+     *
+     * @param physicalTables  A set of <tt>PhysicalTable</tt>'s
+     * @param columns  The columns associated with all tables


In what way are they associated? Do all tables need to have these columns? Is this set a superset of all columns? etc.

Perhaps we should describe this as 'The set of columns supplied by this availability'

cdeszaq · 2017-03-23T20:42:55Z

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

+/**
+ * An implementation of availability which which joins physical tables with the same dimension schema
+ * but non-intersecting metric schemas. Metrics on the <tt>MetricUnionAvailability</tt> are sourced from
+ * exactly one of the participating tables.


So, if a table has an availability, but an availability is joining tables, do we have a circle in this dependency? Should availabilities really only do joins of availabilities, or am I confused? @michael-mclawhorn or @garyluoex, can you shed more life on this?

'physical tables' here is shorthand for the availabilities of physical tables, I suppose.

I would vote for using the longer but more descriptive description, to remove confusion and ambiguity. 5 months from now, when we forget how this is architected, we should be able to read this and understand what is going on.

cdeszaq · 2017-03-23T20:45:54Z

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

+    public MetricUnionAvailability(@NotNull Set<PhysicalTable> physicalTables, @NotNull Set<Column> columns) {
+        metricColumns =  columns.stream()
+                .filter(column -> column instanceof MetricColumn)
+                .map(column -> (MetricColumn) column)


Perhaps use the Utils::getSubsetByType helper method here instead of doing it yourself?

cdeszaq · 2017-03-23T21:14:10Z

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

+                .collect(
+                        Collectors.toMap(
+                                entry -> entry.getKey(),
+                                entry -> entry.getValue(),


Can be method references

cdeszaq · 2017-03-23T21:15:32Z

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

+    @Override
+    public Map<Column, List<Interval>> getAllAvailableIntervals() {
+        return metricAvailability.keySet().stream()
+                .map(availability -> availability.getAllAvailableIntervals().entrySet())


Can be decomposed to 2 map calls:

.map(Availability::getAllAvailableIntervals) .map(Map::entrySet)

cdeszaq · 2017-03-23T21:19:22Z

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

+                                    List<Interval> combined = new ArrayList<>();
+                                    combined.addAll(value1);
+                                    combined.addAll(value2);
+                                    return combined;


I think this body can be simpler, not needing the extra list construction:

value1.addAll(value2); return value1;

cdeszaq · 2017-03-23T21:21:22Z

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

+
+    private final Set<TableName> dataSourceNames;
+    private final Set<MetricColumn> metricColumns;
+    private final Map<Availability, Set<MetricColumn>> metricAvailability;


Can we give thses a name that indicates what these things mean rather than what these things are? For example, when metricColumns shows up in the code of this class, it's often unclear what this collection is being used for, and a better name would help that.

cdeszaq · 2017-03-23T21:47:22Z

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

+     * @param allColumnNames  Set of all column names
+     * @param apiFilters  Map of dimension to its set of API filters
+     */
+    public DataSourceConstraint(


If this is only supposed to be used within this class, it should be protected, not public.

@michael-mclawhorn This constructor is a copy-constructor that doesn't go through the same defensive copies that the other constructor does. It's only purpose in life is to support the with* method that this PR added, and should not be called from outside this class.

cdeszaq · 2017-03-28T15:32:36Z

The branch this had been based on got merged and deleted, which auto-closed this PR. I've re-set it's base to master and reopened it.

michael-mclawhorn

I need some clarifications about some of the testing.

michael-mclawhorn · 2017-03-29T20:34:07Z

CHANGELOG.md

+
+- [MetricUnionAvailability](https://github.com/yahoo/fili/pull/193)
+    * Added `MetricUnionAvailability`  which joins physical tables with the same dimension schema
+    but non-intersecting metric schemas. Metrics on the `MetricUnionAvailability` are sourced from exactly one of the participating tables.


Extra space.

michael-mclawhorn · 2017-03-29T20:35:07Z

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

+     * @param name  Name of the physical table as TableName, also used as fact table name
+     * @param columns  The columns for this table
+     * @param physicalTables  A set of <tt>PhysicalTable</tt>'s
+     * @param logicalToPhysicalColumnNames  Mappings from logical to physical names


apostrophe (') should not be here.

michael-mclawhorn · 2017-03-29T20:37:36Z

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

+
+    /**
+     * Returns the coarsest <tt>ZonedTimeGrain</tt> among a set of <tt>PhysicalTables</tt>'s.
+     * <p>


No apostophe.
Maybe say 'the coarsest...that satisfies all tables'

michael-mclawhorn · 2017-03-29T20:37:44Z

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

+     * Returns the coarsest <tt>ZonedTimeGrain</tt> among a set of <tt>PhysicalTables</tt>'s.
+     * <p>
+     * If the set of <tt>PhysicalTables</tt>'s is empty or the coarsest <tt>ZonedTimeGrain</tt> is not
+     * compatible with any of the <tt>PhysicalTables</tt>'s, throw <tt>IllegalArgumentException</tt>.


No apostrophe.

michael-mclawhorn · 2017-03-29T20:38:10Z

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

+     *
+     * @param physicalTables  A set of <tt>PhysicalTable</tt>'s among which the coarsest <tt>ZonedTimeGrain</tt>
+     * is to be returned.
+     *


No apostophe.

michael-mclawhorn · 2017-03-29T21:13:58Z

...ore/src/test/groovy/com/yahoo/bard/webservice/table/resolver/DataSourceConstraintSpec.groovy

+class DataSourceConstraintSpec extends Specification {
+
+    def "test withIntersectedColumnNames"() {
+        given:


Use @unroll and use #arguments in test definition to explain the parameters of each test case:
e.g.
#metricNames intersected with #other produces #newMetricNames

ps @cdeszaq hates the use of 'Test' in test names.

michael-mclawhorn · 2017-03-29T21:42:26Z

.../test/groovy/com/yahoo/bard/webservice/table/availability/MetricUnionAvailabilitySpec.groovy

+        interval2 = new Interval('2018-02-01/2018-03-01')
+        interval3 = new Interval('2019-01-01/2019-02-01')
+        interval4 = new Interval('2018-11-01/2018-12-01')
+


I'm not sure what to make of these intervals and the order of the intervals listed here.

Did you mean you were confused about how they were used?

Sort of. They aren't ordered. And they don't overlap at all. So they feel kind of arbitrary. Is there some meaning that they have relative to each other?

michael-mclawhorn · 2017-03-29T21:45:51Z

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

+     * @return A mapping from <tt>Availability</tt> to <tt>DataSourceConstraint</tt> with non-empty metric names
+     */
+    private Map<Availability, DataSourceConstraint> constructSubConstraint(DataSourceConstraint constraint) {
+        return availabilityToAvailableColumns.entrySet().stream()


This variable should be plural

michael-mclawhorn · 2017-03-29T21:46:54Z

.../test/groovy/com/yahoo/bard/webservice/table/availability/MetricUnionAvailabilitySpec.groovy

+    }
+
+    def "test getAvailableIntervals"() {
+        given:


This test tests only a path with no overlap. Would it make sense to also test when there is an overlap?

michael-mclawhorn · 2017-03-29T21:55:59Z

.../test/groovy/com/yahoo/bard/webservice/table/availability/MetricUnionAvailabilitySpec.groovy

+    }
+
+    def "dataSourceNames aggregates from availabilities not physical tables in MetricUnionAvailability constructor"() {
+        given:


I'm not sure I understand from the name what this test is testing.

michael-mclawhorn

It's pretty much there.
I'm not sure I agree with the implementations you use for the toString() on the availabilities. Let's talk about that offline.
Get rid of the .get(0) thing by mapping down to grains. And if you can, clarify by name the meanings of the intervals in the test.

cdeszaq

Nothing big. Just a couple of very small things. I'm good with this once those get a little love.

cdeszaq · 2017-04-11T22:40:56Z

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

+                logicalToPhysicalColumnNames,
+                new MetricUnionAvailability(physicalTables, columns)
+        );
+        verifyGrainSatisfiesAllTables(IntervalUtils.getCoarsestTimeGrain(physicalTables).get(), physicalTables);


Rather than re-computing the grain, just get it from the parent, since it's already built at this point: getSchema().getTimeGrain()

cdeszaq · 2017-04-12T15:12:01Z

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

+                                                Utils.getSubsetByType(
+                                                        availability
+                                                                .getAllAvailableIntervals()
+                                                                .keySet(),


Doesn't need to wrap

/NotABlocker

cdeszaq · 2017-04-12T16:11:10Z

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

+     * Intervals of the same metric column are associated with the same metric column key. Overlapping intervals under
+     * the same metric column key are collapsed into single interval.
+     *
+     * @return a map of metric column to all of its available intervals in union


This javadoc still needs to be updated to reflect that it's more than just metric column availabilities that are being returned.

michael-mclawhorn · 2017-04-12T18:17:34Z

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

+ * }
+ * </pre>
+ * Note that the joining availabilities must have completely different set of metric columns.
+ */


I would reword this and put it higher. I would say that metric columns available on one availability must not exist on any other availabilities.

michael-mclawhorn · 2017-04-12T20:15:21Z

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

+     * @return duplicate values of <tt>MetricColumn</tt>s
+     */
+    private static Map<MetricColumn, Set<Availability>> getDuplicateValue(
+            Map<Availability, Set<MetricColumn>> availabilityToAvailableColumns


The should probably be getDuplicateValues (plural)

garyluoex

Minor suggestions, feel free to change.

garyluoex · 2017-04-12T19:39:55Z

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

+        super(
+                name,
+                IntervalUtils.getCoarsestTimeGrain(physicalTables).orElseThrow(() -> {
+                    String message = "At least 1 physical table needs to be provided";


We need a better error message, at least note the current table name.

garyluoex · 2017-04-12T19:41:24Z

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

+                IntervalUtils.getCoarsestTimeGrain(physicalTables).orElseThrow(() -> {
+                    String message = "At least 1 physical table needs to be provided";
+                    LOG.error(message);
+                    return new IllegalArgumentException(message);


Just a note, it is not clear why to throw a illegal argument here, who is receiving invalid argument? which is a good indicator of something is not right. But I am fine leaving it here.

garyluoex · 2017-04-12T19:45:41Z

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

+                .map(PhysicalTable::getSchema)
+                .map(PhysicalTableSchema::getTimeGrain)
+                .allMatch(grain -> grain.satisfiedBy(coarsestTimeGrain))) {
+            String message = String.format("There is no mutually satisfying grain among: %s", physicalTables);


Should probably note the current physical table name in the message.

garyluoex · 2017-04-12T19:46:50Z

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

+                .allMatch(grain -> grain.satisfiedBy(coarsestTimeGrain))) {
+            String message = String.format("There is no mutually satisfying grain among: %s", physicalTables);
+            LOG.error(message);
+            throw new IllegalArgumentException(message);


It is not really Illegal argument, but argument that is legal but does not make sense are given. I think simply runtime exception is better.

garyluoex · 2017-04-12T19:50:10Z

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

+ * +---------------+---------------+
+ * | metricColumn1 | metricColumn2 |
+ * +---------------+---------------+
+ * | [1/10]        | [20/30]       |


it is not clear what these numbers are when looking at it, it would be nice if it is actually time interval instead of number interval

garyluoex · 2017-04-12T20:12:44Z

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

+        Map<MetricColumn, Set<Availability>> duplicates = getDuplicateValue(availabilitiesToAvailableColumns);
+        if (!duplicates.isEmpty()) {
+            String message = String.format(
+                    "While constructing MetricUnionAvailability, Metric columns are not unique - %s",


Should attach table name into the message.

garyluoex · 2017-04-12T22:05:19Z

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

+        return new SimplifiedIntervalList(
+                constructSubConstraint(constraints).entrySet().stream()
+                        .map(entry -> entry.getKey().getAvailableIntervals(entry.getValue()))
+                        .map(i -> (Set<Interval>) new HashSet<>(i))


what is i? Does method reference HashSet::new work?

garyluoex · 2017-04-12T22:19:55Z

.../test/groovy/com/yahoo/bard/webservice/table/availability/MetricUnionAvailabilitySpec.groovy

+        availability2 = Mock(Availability)
+
+        availability1.getDataSourceNames() >> Sets.newHashSet(TableName.of('source1'))
+        availability2.getDataSourceNames() >> Sets.newHashSet(TableName.of('source2'))


I think it turns out that if you wrap the right part with a parenthesis, things will work.

michael-mclawhorn

There are a few tests that probably should be added here.

I would be willing add some if you wanted to migrate the work to a branch in the main github.

michael-mclawhorn · 2017-04-12T21:54:56Z

.../test/groovy/com/yahoo/bard/webservice/table/availability/MetricUnionAvailabilitySpec.groovy

+    def "checkDuplicateValue returns duplicate metric column names or empty, if no duplicates, from 2 physical tables in the case of #caseDescription"() {
+        when:
+        Map<MetricColumn, Set<Availability>> actual = MetricUnionAvailability.getDuplicateValue(
+                [


Missed the rename

michael-mclawhorn · 2017-04-12T21:57:45Z

.../test/groovy/com/yahoo/bard/webservice/table/availability/MetricUnionAvailabilitySpec.groovy

+
+        expect:
+        metricUnionAvailability.constructSubConstraint(dataSourceConstraint).size() == 2
+        metricUnionAvailability.getAvailableIntervals(dataSourceConstraint) == new SimplifiedIntervalList(


constructSubConstraint here isn't being tested for trimming down columns. You should add a column to it that gets filtered.

michael-mclawhorn · 2017-04-12T22:00:10Z

.../test/groovy/com/yahoo/bard/webservice/table/availability/MetricUnionAvailabilitySpec.groovy

+        ['2017-01-01/2017-02-01'] | ['2017-01-01/2017-01-15'] | ['2017-01-01/2017-01-15'] | "full back overlap (0/10, 0/5)"
+        ['2017-01-01/2017-02-01'] | ['2017-01-15/2017-01-25'] | ['2017-01-15/2017-01-25'] | "fully contain (0/10, 3/9)"
+    }
+}


We should test that the subconstraints are correctly applied to distinct subavailabilities.

e.g. Have SubAvailability-A (metric1, metric2) , SubAvailability-B (metric3, metric4) and test that DSS with columns (metric1, metric3) calls A with metric1 and B with metric3.

garyluoex added NEED 2 REVIEWS REVIEWABLE labels Mar 10, 2017

garyluoex requested changes Mar 10, 2017

View reviewed changes

garyluoex added the NEED CHANGES label Mar 10, 2017

QubitPi commented Mar 14, 2017

View reviewed changes

garyluoex removed the NEED CHANGES label Mar 16, 2017

michael-mclawhorn requested changes Mar 16, 2017

View reviewed changes

garyluoex force-pushed the UDCore3 branch 2 times, most recently from 74b324b to f046ee2 Compare March 17, 2017 00:04

cdeszaq requested changes Mar 23, 2017

View reviewed changes

QubitPi changed the title ~~Implement MetricUnionAvailability~~ Implement MetricUnionAvailability & MetricUnionCompositeTable Mar 24, 2017

garyluoex force-pushed the UDCore3 branch from 0a624be to a08bdd8 Compare March 24, 2017 21:23

QubitPi changed the base branch from UDCore3 to FieldTagging March 24, 2017 21:30

QubitPi changed the base branch from FieldTagging to UDCore3 March 24, 2017 21:30

garyluoex force-pushed the UDCore3 branch 2 times, most recently from 4c65f95 to a36562f Compare March 24, 2017 21:51

michael-mclawhorn mentioned this pull request Mar 27, 2017

Pr/193 #201

Closed

QubitPi changed the base branch from UDCore3 to pr/190 March 27, 2017 22:19

michael-mclawhorn closed this Mar 28, 2017

cdeszaq changed the base branch from pr/190 to master March 28, 2017 15:32

cdeszaq reopened this Mar 28, 2017

QubitPi mentioned this pull request Mar 29, 2017

Implement MetricUnionAvailability and MetricUnionCompositeTable #206

Closed

QubitPi closed this Mar 29, 2017

QubitPi reopened this Mar 29, 2017

QubitPi changed the base branch from master to MoreSubstantiveApiMetricNameChange March 29, 2017 18:25

QubitPi changed the base branch from MoreSubstantiveApiMetricNameChange to master March 29, 2017 18:25

michael-mclawhorn requested changes Mar 29, 2017

View reviewed changes

michael-mclawhorn requested changes Mar 30, 2017

View reviewed changes

QubitPi self-assigned this Apr 11, 2017

garyluoex assigned cdeszaq and unassigned QubitPi Apr 11, 2017

cdeszaq approved these changes Apr 12, 2017

View reviewed changes

cdeszaq added NEED 1 REVIEW and removed NEED 2 REVIEWS labels Apr 12, 2017

QubitPi added 11 commits April 12, 2017 13:08

Implement MetricUnionAvailability and MetricUnionCompositeTable

35e1695

address @michael-mclawhorn's comments

6ef90c3

address comments

39b0d43

update

41d6cb2

update CHANGELOG

6c40b86

format javadoc

7a1ba89

update javadoc

4c18c26

update to use the new method for finding coarsest ZonedTimeGrain

1783b15

address @cdeszaq's comments

c2fe642

address @cdeszaq's comments

910b777

address @cdeszaq's comments

60ddc1b

michael-mclawhorn reviewed Apr 12, 2017

View reviewed changes

address @michael-mclawhorn's comments

d72d1e8

michael-mclawhorn reviewed Apr 12, 2017

View reviewed changes

address @michael-mclawhorn's comments

e859d34

garyluoex approved these changes Apr 12, 2017

View reviewed changes

address @garyluoex's comments

8496f62

QubitPi removed NEED 1 REVIEW REVIEWABLE labels Apr 13, 2017

michael-mclawhorn requested changes Apr 13, 2017

View reviewed changes

cdeszaq merged commit 61999bb into yahoo:master Apr 13, 2017

cdeszaq mentioned this pull request Apr 13, 2017

Insufficient tests for MetricUnionAvailability's constructSubConstraints method and related calls #238

Closed

QubitPi deleted the UDCore3-implement-MetricUnionAvailability branch April 13, 2017 16:28

QubitPi added the MERGEABLE label Jul 19, 2017


		class TestMetricUnionAvailability extends MetricUnionAvailability {

		List<TestAvailability> testAvailabilities

Implement MetricUnionAvailability & MetricUnionCompositeTable #193

Implement MetricUnionAvailability & MetricUnionCompositeTable #193

Conversation

QubitPi commented Mar 10, 2017 • edited Loading

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

QubitPi commented Mar 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

QubitPi Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

garyluoex Mar 16, 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

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

cdeszaq commented Mar 28, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi Mar 30, 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

michael-mclawhorn left a 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

garyluoex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi commented Mar 10, 2017 •

edited

Loading

QubitPi commented Mar 14, 2017 •

edited

Loading

QubitPi Mar 16, 2017 •

edited

Loading

garyluoex Mar 16, 2017 •

edited

Loading

QubitPi Mar 30, 2017 •

edited

Loading