Add PermissiveAvailability & PermissiveConcretePhysicalTable #190

QubitPi · 2017-03-06T23:10:03Z

No description provided.

garyluoex · 2017-03-06T23:18:02Z

Need CHANGELOG.md, also add a test to test PermissiveConcretePhysicalTable correctly creates PermissiveAvailability and that the getAvailableIntervals and getAllAvailableIntervals uses the PermissiveAvailability appropriately.

cdeszaq

I only evaluated this from GitHub (haven't pulled the code locally yet), so I didn't look at anything deeper that surface-code (so, no inspections on extension vs. implementation, etc.), but I wanted to get this level of feedback to you so you could start looking at it.

I'll dive a bit deeper shortly.

cdeszaq · 2017-03-07T15:17:25Z

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

+            @NotNull ZonedTimeGrain timeGrain,
+            @NotNull Set<Column> columns,
+            @NotNull Map<String, String> logicalToPhysicalColumnNames,
+            @NotNull Availability availability


This is the exact same constructor as exists in ConcretePhysicalTable, and there are otherwise no methods in this class that are not directly from the parent. What added value is this class giving us?

As a minimum, I would expect that the constructor here would require a PermissiveAvailability, rather than any Availability, otherwise there's no assertion that this PhysicalTable is going to do anything "permissive".

cdeszaq · 2017-03-07T15:18:51Z

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

+import java.util.Set;
+
+/**
+ * An instance of permissive Physical table that is backed by a single fact table.


Unless a class is set up as a singleton, it isn't really an instance. It may be an implementation, which is perhaps what you meant?

Since this is a specialization of a ConcretePhysicalTable, the JavaDoc on this class should talk about what is different about this implementation. Why does this class exist, what is it for, what concepts does it add on top of what ConcretePhysicalTable already does.

To put it a different way, merely adding (essentially) the class name to the javadoc of the parent class doesn't really make this a valuable / useful comment. 😄

cdeszaq · 2017-03-07T15:23:51Z

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

+import java.util.stream.Collectors;
+
+/**
+ * An availability which doesn't force immutability on its contents.


Is it really the case that "permissive" means "mutable"? My understanding was that "permissive" in this context has more to do with something related to how intervals from multiple columns were counted, to determine if all columns were available or not. It's entirely possible I'm misunderstanding something, but if mutability is really the question here, I think we should rename this class to better reflect that.

cdeszaq · 2017-03-07T15:24:55Z

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

+
+    @Override
+    public Set<TableName> getDataSourceNames() {
+        return Collections.singleton(name);


Rather than make this wrapping every time getDataSourceNames is called, we should do it only once in the constructor and store the collection.

cdeszaq · 2017-03-07T15:27:56Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

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


cdeszaq · 2017-03-07T15:31:15Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        expect:
+        permissiveAvailability.getAllAvailableIntervals() == [
+                (column1): [interval1].toSet(),
+                (column2): [interval2].toSet()


again, using as Set is generally better

cdeszaq · 2017-03-07T15:38:01Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        dataSourceConstraint.getAllColumnNames() >> ['column1', 'column2']
+
+        expect:
+        permissiveAvailability.getAvailableIntervals(dataSourceConstraint) == new SimplifiedIntervalList([interval1])


This test seems to be wrong. interval1 is not the intersection of the intervals for the two columns, it's the union.

Also, additional test cases for many more overlap / non-overlap cases would be good to include here. In particular:

full overlap (start/end, start/end)

0 overlap (-10/-1, 0/10)

0 overlap abutting (-10/0, 0/10)

partial front overlap (0/10, 5/15)

partial back overlap (0/10, -5/5)

full front overlap (0/10, 5/10)

full back overlap (0/10, 0/5)

Right, PermissiveAvailability in this case should return the union. This is how I understand "Concrete"(intersection) and "Permissive"(union) availavilities.

cdeszaq · 2017-03-07T15:39:00Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        permissiveAvailability.getAvailableIntervals(dataSourceConstraint) == new SimplifiedIntervalList([interval1])
+    }
+
+    def "getAvailableInterval returns empty interval if given column not configured"() {


not configured where? It's unclear because the setup clearly configures it somewhere.

This ignored interval is not associated with any column in setup. I changed ignored to unConfiguredColumn

cdeszaq · 2017-03-07T15:39:50Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        permissiveAvailability.getAvailableIntervals(dataSourceConstraint) == new SimplifiedIntervalList([interval1])
+    }
+
+    def "getAvailableInterval returns empty interval if given column not configured"() {


Also need to run this same check for getAllAvailableIntervals, since it's behavior should be similar.

Or rather, it shouldn't include columns in the underlying metadata that this availability isn't configured for.

I think this check is done in the "getAllAvailability returns the union of all availabilities for each columns configured to the table" test? (see it in the next commit)

cdeszaq · 2017-03-07T15:41:54Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        permissiveAvailability.getAvailableIntervals(dataSourceConstraint) == new SimplifiedIntervalList()
+    }
+
+    def "getAvailableInterval returns empty interval if given empty column request"() {


Is this the correct behavior? Should this throw an exception instead? It's not clear to me what's right, since "all data is available if I don't need data from any columns" seems like it might be a reasonable response in this case.

The current logic is if you don't ask for anything, you don't get anything, consider the case where the user asks for a column not on the table in which is filtered out in the availability (although this should not be possible).

garyluoex · 2017-03-08T00:08:21Z

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

+     * @param columns  The columns for this table
+     * @param logicalToPhysicalColumnNames  Mappings from logical to physical names
+     * @param permissiveAvailability the instance of Permissive Availability which makes sure that
+     *                               the table is backed by permissive availability


Get rid of the indention on the second line here, use intellij's auto format will help.

garyluoex · 2017-03-08T00:10:13Z

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

+     * @param permissiveAvailability the instance of Permissive Availability which makes sure that
+     *                               the table is backed by permissive availability
+     */
+    protected PermissiveConcretePhysicalTable(


This should be public and does not take a permissiveAvailability as its argument, it should construct the availability and pass it to super.

garyluoex · 2017-03-08T00:11:05Z

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

+import javax.validation.constraints.NotNull;
+
+/**
+ * An availability which allows multiple disconnected intervals(union) on its contents.


Instead of saying multiple disconnected intervals, just say missing intervals might be more appropriate.

garyluoex · 2017-03-08T00:11:56Z

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

+ */
+public class PermissiveAvailability implements Availability {
+
+    private final TableName tableName;


This is not needed if we are going to store a Set<TableName> dataSourceNames

garyluoex · 2017-03-08T00:13:56Z

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

+        this.tableName = tableName;
+        this.columns = columns;
+        this.metadataService = metadataService;
+        dataSourceNames = Collections.singleton(tableName);


Add a this. to be consistent with everything else so that it looks nicer.

garyluoex · 2017-03-08T00:19:11Z

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

+        }
+
+
+        Map<Column, Set<Interval>> x = getAllAvailableIntervals();


x is not acceptable.

Agree. I forgot to remove this debug line.

garyluoex · 2017-03-08T00:31:58Z

...src/test/groovy/com/yahoo/bard/webservice/table/availability/AvailabilityTestingUtils.groovy

+        return map.entrySet().stream()
+                .collect(
+                    Collectors.toMap(
+                            {Map.Entry it -> new Column(it.key)},


can we try remove the type cast and directly use it? since intellij complains anyways, if it compile and runs might as well remove the type cast.

It's not a cast in this context, it's a type indication, but otherwise you are correct, Groovy's dynamic typing means that it's not needed here (unless we wanted it for clarity)

garyluoex · 2017-03-08T00:32:34Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+    Column column1
+    Column column2
+    Interval interval1
+    Interval interval2


Grouping the ones with the same type into a single line make things look cleaner

Actually, I generally hate lumping things onto one line. It may just be my personal preference, but it makes it harder to see how many things there are, and harder to see what changes if variable names need to change. It also makes it more difficult to initialize variables when they are defined.

It's not a big deal, but I definitely would rather have more lines, with each definition on it's own line, rather than not.

And for reference, here's the relevant style guide indication for this: https://google.github.io/styleguide/javaguide.html#s4.8.2-variable-declarations

garyluoex · 2017-03-08T00:38:06Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        permissiveAvailability.getAvailableIntervals(dataSourceConstraint) == new SimplifiedIntervalList()
+    }
+
+    def "getAvailableInterval returns empty interval if given empty column request"() {


The current logic is if you don't ask for anything, you don't get anything, consider the case where the user asks for a column not on the table in which is filtered out in the availability (although this should not be possible).

garyluoex · 2017-03-08T00:39:36Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        column2 = new Column('column2')
+
+        interval1 = new Interval('2010-01-01/2020-12-31')
+        interval2 = new Interval('2017-01-01/2020-12-31')


Move the setup things that are not used by more than one test into the single test that uses it.

garyluoex · 2017-03-08T15:58:25Z

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

+        Set<String> allColumnNames = constraints.getAllColumnNames();
+
+        if (allColumnNames.isEmpty()) {
+            throw new IllegalArgumentException("DataSourceConstraint needs to have at least 1 constraint column");


Follow the behavior if things are empty in ConcreteAvailability, we do not want different behavior for each Availability.

garyluoex

Last two things, otherwise good

garyluoex · 2017-03-08T16:24:54Z

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

+        Map<String, Set<Interval>> allAvailableIntervals = metadataService.getAvailableIntervalsByTable(
+                dataSourceNames.stream()
+                        .findFirst()
+                        .orElseThrow(NullPointerException::new)


This is not necessary since the constructor only allows a single name to be passed in, also throwing a null pointer exception with out any additional information is not recommended.

garyluoex · 2017-03-08T16:29:46Z

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

@@ -68,17 +70,12 @@ public PermissiveAvailability(
    public SimplifiedIntervalList getAvailableIntervals(DataSourceConstraint constraints) {
        Set<String> allColumnNames = constraints.getAllColumnNames();

-        if (allColumnNames.isEmpty()) {


Refer to ConcreteAvailability the behavior of getAvailableIntervals behavior, it should filter out to only the columns that is configured in this table. Should add a test to ensure this behavior.

QubitPi · 2017-03-08T17:09:41Z

@garyluoex @cdeszaq More comments welcome :)

cdeszaq

Overall, pretty good. I think there can be a lot of code deduplication between ConcreteAvailability / PermissiveAvailability (which I think should be renamed to PermissiveConcreteAvailability to match the PhysicalTable naming pattern.)

I left a bunch of comments along those DRY lines, but I also did a bunch of playing with the code to see where it could go. Rather than throw that away, here's what I ended up with for those classes. Feel free to take what you think is good from them:

public class PermissiveAvailability extends ConcreteAvailability {

    /**
     * Constructor.
     *
     * @param tableName The name of the data source
     * @param columns A set of columns associated with the data source
     * @param metadataService A service containing the data source segment data
     */
    public PermissiveAvailability(
            @NotNull TableName tableName,
            @NotNull Set<Column> columns,
            @NotNull DataSourceMetadataService metadataService
    ) {
        super(tableName, columns, metadataService);
    }

    @Override
    public SimplifiedIntervalList getAvailableIntervals(DataSourceConstraint constraints) {
        Set<String> requestColumns = filterRequestColumns(constraints);
        return getIntervalsForColumnNames(requestColumns)
                .flatMap(Set::stream)
                .collect(SimplifiedIntervalList.getCollector());
    }
}

public class ConcreteAvailability implements Availability {

    private final TableName name;
    private final Set<TableName> names;
    private final Set<Column> columns;
    private final DataSourceMetadataService metadataService;
    private final Set<String> columnNames;

    /**
     * Constructor.
     *
     * @param tableName The name of the data source associated with this ImmutableAvailability
     * @param columns The columns associated with the table and availability
     * @param metadataService A service containing the datasource segment data
     */
    public ConcreteAvailability(
            TableName tableName,
            @NotNull Set<Column> columns,
            @NotNull DataSourceMetadataService metadataService
    ) {
        this.name = tableName;
        this.metadataService = metadataService;
        this.names = Collections.singleton(tableName);
        this.columns = columns;
        this.columnNames = columns.stream()
                .map(Column::getName)
                .collect(Collectors.toSet());
    }

    protected Set<Column> getColumns() {
        return columns;
    }

    protected DataSourceMetadataService getMetadataService() {
        return metadataService;
    }

    @Override
    public Set<TableName> getDataSourceNames() {
        return names;
    }

    @Override
    public Map<Column, Set<Interval>> getAllAvailableIntervals() {

        Map<String, Set<Interval>> allAvailableIntervals = getAllAvailableIntervalsByName();
        return columns.stream()
                .collect(
                        Collectors.toMap(
                                Function.identity(),
                                column -> allAvailableIntervals.getOrDefault(column.getName(), Collections.emptySet())
                        )
                );
    }

    @Override
    public SimplifiedIntervalList getAvailableIntervals(DataSourceConstraint constraints) {
        Set<String> requestColumns = filterRequestColumns(constraints);
        return new SimplifiedIntervalList(
                getIntervalsForColumnNames(requestColumns)
                        .reduce(null, IntervalUtils::getOverlappingSubintervals)
        );
    }

    protected Set<String> filterRequestColumns(DataSourceConstraint constraints) {
        return constraints.getAllColumnNames().stream()
                .filter(columnNames::contains)
                .collect(Collectors.toSet());
    }

    protected Map<String, Set<Interval>> getAllAvailableIntervalsByName() {
        return metadataService.getAvailableIntervalsByTable(name);
    }

    protected Stream<Set<Interval>> getIntervalsForColumnNames(Set<String> columnNames) {
        if (columnNames.isEmpty()) {
            return Stream.empty();
        }

        Map<String, Set<Interval>> allAvailableIntervals = getAllAvailableIntervalsByName();
        return columnNames.stream()
                .map(columnName -> allAvailableIntervals.getOrDefault(columnName, Collections.emptySet()));
    }
}

cdeszaq · 2017-03-09T22:33:04Z

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

+ * An implementation of permissive Physical table that is backed by a single fact table.
+ * <p>
+ * This is different from its parent <tt>ConcretePhysicalTable</tt> by using a different
+ * Availability type, which is <tt>PermissiveAvailability</tt>


This should talk about what it means to use a different type of availability. In particular, pointing out which methods of a PhysicalTable are impacted by this would be a good start.

cdeszaq · 2017-03-09T22:33:46Z

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

+import javax.validation.constraints.NotNull;
+
+/**
+ * An availability which allows missing intervals on its contents.


What does this mean?

cdeszaq · 2017-03-09T22:37:10Z

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

+                dataSourceNames.stream()
+                        .findFirst()
+                        .get()
+        );


This method appears to be doing exactly the same thing as ConcreteAvailability with the only difference being we unpack the collection of table names each time. We should not have this duplication. If a PermissiveAvailability is ConcreteAvailability, why not make it so?

Either way, we should not have to unpack this every time, since it's useless work. Instead, just hold the collection and the single table name if we need to.

cdeszaq · 2017-03-09T22:37:51Z

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

+    }
+
+    @Override
+    public SimplifiedIntervalList getAvailableIntervals(DataSourceConstraint constraints) {


This definitely needs a fairly rich comment explaining what getAvailableIntervals does, since this is the meat of this class (ie. it's the reason the class exists)

cdeszaq · 2017-03-09T22:41:24Z

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

+                dataSourceNames.stream()
+                        .findFirst()
+                        .get()
+        );


This seems to be a clear duplicate of work that's being done in the earlier method. Can we de-duplicate the code? Perhaps pull the duplicated code out into a getAvailableIntervals method? (and possibly have this live in the ConcreteAvailability class, since it too seems to do the same thing twice)

cdeszaq · 2017-03-10T17:23:21Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

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


It would cool to have these comments as a string variable and have that expanded into the test name...

@cdeszaq Agree. How do I do that?

One way to do it would be like this:

Bar mockBar = Mock(Bar) @Unroll def "The #thing always does #actionCount #action whenever it is poked and #reason"() { given: "A configured thing and a mock" thing."set$initialParam"(initialState) when: "It gets poked" thing.poke() then: "It has taken the action the correct number of times" actionCount * mockBar."action"() where: thing | actionCount | action | initialParam | initialState | reason new Dog() | 1 | "jump" | "Barking" | true | "it is barking" }

This example is a bit fancy, and shows off much more than how to get a string to unroll as part of the test name, but you can see that the reason where variable is only used as an unrolled parameter in the test name.

In a nutshell, you can put a # in front of any where parameter in the test name when using @Unroll, and the value of that variable will be swapped into the test name when Spock generates the tests (which is on for each iteration in the where block.

Some of the other stuff that's in that example shows off how Groovy will let you use strings as method names :-)

More specifically, for this test, of you make the where block like this:

where: firstInterval | secondInterval | expected | reason '2017-01-01/2017-02-01' | '2017-01-01/2017-02-01' | ['2017-01-01/2017-02-01'] | "full overlap (start/end, start/end)" '2017-01-01/2017-02-01' | '2018-01-01/2018-02-01' | ['2017-01-01/2017-02-01', '2018-01-01/2018-02-01'] | "0 overlap (-10/-1, 0/10)" '2017-01-01/2017-02-01' | '2017-02-01/2017-03-01' | ['2017-01-01/2017-03-01'] | "0 overlap, abutting (-10/0, 0/10)" '2017-01-01/2017-02-01' | '2017-01-15/2017-03-01' | ['2017-01-01/2017-03-01'] | "partial front overlap (0/10, 5/15)" '2017-01-01/2017-02-01' | '2016-10-01/2017-01-15' | ['2016-10-01/2017-02-01'] | "partial back overlap (0/10, -5/5)" '2017-01-01/2017-02-01' | '2017-01-15/2017-02-01' | ['2017-01-01/2017-02-01'] | "full front overlap (0/10, 5/10)" '2017-01-01/2017-02-01' | '2017-01-01/2017-01-15' | ['2017-01-01/2017-02-01'] | "full back overlap (0/10, 0/5)" '2017-01-01/2017-02-01' | '2017-01-15/2017-01-25' | ['2017-01-01/2017-02-01'] | "fully contain (0/10, 3/9)"

Then you can add it to the test name, like this:

getAvailableIntervals returns the union of requested columns when available intervals have #reason

Brilliant! Thanks! I'll take a note.

cdeszaq · 2017-03-10T17:24:30Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        permissiveAvailability.getAvailableIntervals(dataSourceConstraint) == new SimplifiedIntervalList(
+                expected.stream()
+                        .map{it -> new Interval(it)}
+                        .collect(Collectors.toSet())


Can be simplified using groovy: expected.collect{new Interval(it)} as Set

cdeszaq · 2017-03-10T17:25:38Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        permissiveAvailability.dataSourceNames == Collections.singleton(TableName.of('table'))
+    }
+
+    def "getAllAvailability returns all availabilities for each columns configured to the table"() {


method name here in the description doesn't seem to match the method under test

cdeszaq · 2017-03-10T17:27:46Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        permissiveAvailability.dataSourceNames == Collections.singleton(TableName.of('table'))
+    }
+
+    def "getAllAvailability returns all availabilities for each columns configured to the table"() {


We should also have a test like this for only returning availabilities for columns that are configured on the Availability, even if they are in the underlying DataSourceMetadataService.

I think the new test is the same as getAvailableInterval returns only configured column

cdeszaq · 2017-03-10T17:34:12Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+
+import java.util.stream.Collectors
+
+class PermissiveAvailabilitySpec extends Specification  {


If PermissiveAvailability extends from ConcreteAvailability, then this Spec only needs to worry about the specific differences in behavior from it's base class, which will likely simplify this Spec significantly.

That said, if the comments I've left here can make the Spec for the parent better, then they should be rolled into the Parent spec.

👍 on removing test that don't test the difference.

cdeszaq

Partial. Still need to dig into most recent test changes.

cdeszaq · 2017-03-13T16:36:51Z

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

        this.metadataService = metadataService;
+        this.names = Collections.singleton(tableName);
+        this.columns = columns;


Should turn this into an ImmutableSet so that callers can't modify it if they get it via getColumns

cdeszaq · 2017-03-13T16:37:05Z

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

+        this.columns = columns;
+        this.columnNames = columns.stream()
+                .map(Column::getName)
+                .collect(Collectors.toSet());


Should also be made into an ImmutableSet, though it matters a little less, since it's never returned directly. Better to be safe though.

cdeszaq

Other than the couple of final things, this is looking really good. Just a few small cleanups!

cdeszaq · 2017-03-13T18:32:02Z

...e/src/test/groovy/com/yahoo/bard/webservice/table/PermissiveConcretePhysicalTableSpec.groovy

+    }
+
+    @Unroll
+    def "The spanning intervals are returned even when individual columns are not fully covering the interval"() {


@Unroll tests should have some sort of where expansion, to differentiate the test iterations and indicate what is being tested in each iteration.

I though it makes more sense to remove @Unroll :)

If we have a data-driven test (ie. a test with a where block), we should always use @Unroll. The reason is that @Unroll makes a test (as seen by the JUnit Test Runner) for each "row" in a where table. Without that, all of the iterations in the where block are run as a single test, so a failure with the 1st row marks the entire test as a failure, instead of allowing each iteration to fail on it's own, which gives much more information.

michael-mclawhorn

Looking pretty good.
I like the readability of the tests particularly.

michael-mclawhorn · 2017-03-13T22:43:18Z

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

@@ -25,8 +26,10 @@
 public class ConcreteAvailability implements Availability {

    private final TableName name;
+    private final Set<TableName> names;


Let's align the variable name with the getter name. e.g. dataSourceNames

michael-mclawhorn · 2017-03-14T21:17:44Z

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

+
+/**
+ * An implementation of permissive Physical table that is backed by a single fact table.
+ * <p>


A permissive table that is backed by a single fact table or a concrete table with permissive availability? I would have thought the latter.

michael-mclawhorn · 2017-03-14T21:37:31Z

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

+        return getIntervalsForColumnNames(requestColumns)
+                .flatMap(Set::stream)
+                .collect(SimplifiedIntervalList.getCollector());
+    }


The Permissive availability shouldn't use the constraints to limit the available times.
Instead the columns from the Availability should be used.

michael-mclawhorn · 2017-03-14T21:41:55Z

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

                        .reduce(null, IntervalUtils::getOverlappingSubintervals)
        );
    }
+


Technically, I believe this could return a null pointer exception.
The way to fix this would be to empty check the request columns before doing the stream and returning an empty SIL

Another option:

return new SimplifiedIntervalList( requestColumns.isEmpty() ? Collections.emptySet() : getIntervalsForColumnNames(requestColumns) .reduce(null, IntervalUtils::getOverlappingSubintervals) );

michael-mclawhorn · 2017-03-14T21:59:42Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+
+import java.util.stream.Collectors
+
+class PermissiveAvailabilitySpec extends Specification  {


👍 on removing test that don't test the difference.

cdeszaq

Once that small tweak is addressed, this is good to go. So, after 1 more commit, this will be squashed and merged.

cdeszaq · 2017-03-15T20:47:50Z

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

+     */
+    @Override
+    public SimplifiedIntervalList getAvailableIntervals(DataSourceConstraint constraints) {
+        return getIntervalsForColumnNames(getColumns().stream().map(Column::getName).collect(Collectors.toSet()))


Rather than recompute this, we should pull this collection from the parent class: https://github.com/yahoo/fili/pull/190/files#diff-6923730d44e29354ccfcc8d1ca622d8eR33

We may need to add a protected getter for it to the parent, but that's where we should use it from.

michael-mclawhorn

The main thing is that the permissive tests are wrong to the feature we're expecting.

michael-mclawhorn · 2017-03-15T15:29:44Z

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

+     * @param columns  The columns for this table
+     * @param logicalToPhysicalColumnNames  Mappings from logical to physical names
+     * @param availability the instance of Availability specifying the availabilities of table columns
+     */


Whitepace: two spaces after param name.

michael-mclawhorn · 2017-03-15T21:56:56Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+    }
+
+    def "getAvailableIntervals returns only configured column"() {
+        given:


grammar 'only configured columns'

This test could be lifted up to ConcreteAvailabilitySpec.

michael-mclawhorn · 2017-03-15T22:09:28Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+        expect:
+        permissiveAvailability.getAvailableIntervals(dataSourceConstraint) == new SimplifiedIntervalList()
+    }
+


These three tests have incorrect results. The reason is not obvious is because you haven't initialized the metadata service they use with actual intervals. They should be returning the full set of intervals supplied by the columns of the permissive table.

michael-mclawhorn

I think these two comments are my last concern.

michael-mclawhorn · 2017-03-16T20:23:55Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+    }
+
+    def "getAvailableIntervals returns empty list of intervals if given empty column request"() {
+        given:


You changed the test behavior but not the test name.

michael-mclawhorn · 2017-03-16T20:24:12Z

...c/test/groovy/com/yahoo/bard/webservice/table/availability/PermissiveAvailabilitySpec.groovy

+    }
+
+    def "getAvailableIntervals returns empty list of intervals if given column not configured"() {
+        given:


You changed the test behavior but not the test name.

michael-mclawhorn · 2017-03-16T20:25:43Z

Once you've got these last changes in, please rebase onto the updated UDCore3 and resolve any conflicts.

There is too much changes to the point that I need to re-review this piece.

…tory

since this needs a rebase onto the underlying PR, but once this is cleanly rebased, I'm happy to give this a quick glance and fast re-approval

michael-mclawhorn · 2017-03-22T00:15:55Z

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

+     * @param columns  The columns for this table
+     * @param logicalToPhysicalColumnNames  Mappings from logical to physical names
+     * @param dataSourceMetadataService data source metadata service containing availability data for the table
+     */


Spacing: two spaces after param name.

…sicalTable

michael-mclawhorn · 2017-03-23T16:43:31Z

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

+
+    public Set<String> getCachedColumnNames() {
+        return cachedColumnNames;
+    }


Let's drop the 'cached' prefix on the member variables in this class and in the getter.

michael-mclawhorn

I think the variable name change will make me happy with this.

cdeszaq

Just a few final things, should be fairly trivial, and the comment updates to reflect the behavior change for Permissive availability vs. it's previous version.

cdeszaq · 2017-03-23T17:30:28Z

...src/test/groovy/com/yahoo/bard/webservice/table/availability/ConcreteAvailabilitySpec.groovy

@@ -99,4 +99,4 @@ class ConcreteAvailabilitySpec extends Specification{
        expect:
        concreteAvailability.getAvailableIntervals(constraint) == new SimplifiedIntervalList()
    }
-}
+}


Newline needed at end of file

cdeszaq · 2017-03-23T17:37:19Z

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

+    @Override
+    public SimplifiedIntervalList getAvailableIntervals(DataSourceConstraint constraints) {
+        Map<String, List<Interval>> allAvailableIntervals = getMetadataService()
+                .getAvailableIntervalsByTable(getName());


This should be using getLatestAvailableIntervalsByTable from the parent class.

Also means that getName from the parent class can go away.

cdeszaq · 2017-03-23T17:48:30Z

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

+     * @return the union of all available intervals
+     */
+    @Override
+    public SimplifiedIntervalList getAvailableIntervals(DataSourceConstraint constraints) {


Since this is unused, we should probably rename the parameter to something like ignored (and probably make sure the javadoc on this method and class explains why it doesn't matter. @michael-mclawhorn should be able to better explain why it doesn't matter if there are questions.

cdeszaq · 2017-03-23T17:49:40Z

CHANGELOG.md

@@ -8,6 +8,9 @@ pull request if there was one.
 Current
 -------
 ### Added:
+- [PermissiveAvailability and PermissiveConcretePhysicalTable](https://github.com/yahoo/fili/pull/190)
+    * Added `PermissiveConcretePhysicalTable` and `PermissiveAvailability` to model table in druid datasource and its availability in the new table availability structure


Since this addition is really a re-add of the old behavior (mostly), just in a new place, we should make sure to call that out. Also, since the behavior is subtly different than the old behavior, we should explain that change in here too. @michael-mclawhorn or @will-lauer can help explain it I think if there are questions.

cdeszaq · 2017-03-23T17:50:51Z

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

+
+    public TableName getName() {
+        return name;
+    }


These two methods are unused if getLatestAvailableIntervalsByTable is used in the permissive availability.

cdeszaq · 2017-03-23T18:03:43Z

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

+        return name;
+    }
+
+    public Set<String> getCachedColumnNames() {


Probably should make this protected, since it's intended as a helper method more than anything else, right?

cdeszaq · 2017-03-23T18:11:37Z

...src/test/groovy/com/yahoo/bard/webservice/table/availability/AvailabilityTestingUtils.groovy

+    static Map<Column, Set<Interval>> constructColumnToIntervalMap(Map<String, Set<String>> columnToIntervalInString) {
+        return columnToIntervalInString.collectEntries{
+            key,
+            value -> [ (new Column(key)): value.collect{new Interval(it)} ]


Typically, format for Groovy closures is to put the closure params on the opening line, and then wrap after that:

[:].collectEntries {key, value -> [(key): value] }

michael-mclawhorn

👍

cdeszaq · 2017-03-27T22:08:30Z

Closing in favor of #200, which is the rebased version of this.

garyluoex added NEED 2 REVIEWS REVIEWABLE labels Mar 6, 2017

cdeszaq requested changes Mar 7, 2017

View reviewed changes

garyluoex added NEED CHANGES and removed NEED CHANGES labels Mar 7, 2017

garyluoex requested changes Mar 8, 2017

View reviewed changes

garyluoex reviewed Mar 8, 2017

View reviewed changes

garyluoex requested changes Mar 8, 2017

View reviewed changes

garyluoex previously approved these changes Mar 8, 2017

View reviewed changes

garyluoex added NEED 1 REVIEW and removed NEED 2 REVIEWS labels Mar 8, 2017

cdeszaq requested changes Mar 10, 2017

View reviewed changes

cdeszaq requested changes Mar 13, 2017

View reviewed changes

michael-mclawhorn requested changes Mar 14, 2017

View reviewed changes

cdeszaq previously approved these changes Mar 15, 2017

View reviewed changes

michael-mclawhorn requested changes Mar 15, 2017

View reviewed changes

michael-mclawhorn requested changes Mar 16, 2017

View reviewed changes

garyluoex force-pushed the UDCore3 branch from 540c757 to 74b324b Compare March 16, 2017 23:56

Integrate new core composite table and availability structure into Fili

f046ee2

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

QubitPi changed the base branch from UDCore3 to JekyllSync March 17, 2017 15:32

QubitPi changed the base branch from JekyllSync to UDCore3 March 17, 2017 15:32

fix rebase problems

6cf1900

garyluoex added 2 commits March 19, 2017 20:35

Modified according to andrew's comment and myself

25ce776

Add a hook for getting dimension dictionary in application binder fac…

31b8cd0

…tory

garyluoex added 2 commits March 20, 2017 18:32

Modify according to andrew and rick's comments

18c09ed

Modified according to most of rick's comments

0cf7882

michael-mclawhorn modified the milestone: mclawhor Mar 22, 2017

michael-mclawhorn changed the base branch from UDCore3 to SegmentMetadataLoaderBindingBug March 22, 2017 00:12

michael-mclawhorn changed the base branch from SegmentMetadataLoaderBindingBug to UDCore3 March 22, 2017 00:13

michael-mclawhorn reviewed Mar 22, 2017

View reviewed changes

garyluoex and others added 2 commits March 23, 2017 10:38

Modified according to more rick's comments

1e39b57

Add implementations of PermissiveAvailability & PermissiveConcretePhy…

429cacc

…sicalTable

michael-mclawhorn reviewed Mar 23, 2017

View reviewed changes

michael-mclawhorn approved these changes Mar 23, 2017

View reviewed changes

michael-mclawhorn requested changes Mar 23, 2017

View reviewed changes

cdeszaq requested changes Mar 23, 2017

View reviewed changes

QubitPi added 2 commits March 23, 2017 16:12

address @michael-mclawhorn's comment

0c7561b

address @cdeszaq's comments

1a0bf08

michael-mclawhorn approved these changes Mar 23, 2017

View reviewed changes

cdeszaq approved these changes Mar 24, 2017

View reviewed changes

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

cdeszaq changed the base branch from UDCore3 to master March 27, 2017 17:05

michael-mclawhorn mentioned this pull request Mar 27, 2017

Add implementations of PermissiveAvailability & PermissiveConcretePhy… #200

Merged

cdeszaq closed this Mar 27, 2017


		import java.util.stream.Collectors

		class PermissiveAvailabilitySpec extends Specification {

Add PermissiveAvailability & PermissiveConcretePhysicalTable #190

Add PermissiveAvailability & PermissiveConcretePhysicalTable #190

Conversation

QubitPi commented Mar 6, 2017

garyluoex commented Mar 6, 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

QubitPi Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

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

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

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 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

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

QubitPi Mar 7, 2017 •

edited

Loading

QubitPi Mar 7, 2017 •

edited

Loading

QubitPi Mar 8, 2017 •

edited

Loading