aha

CHANGELOG.md

@@ -8,6 +8,9 @@ pull request if there was one.
 Current
 -------
 ### Added:
+- [Fix to use physical name instead of logical name to retrieve available interval](https://github.com/yahoo/fili/pull/226)
+    * Added `PhysicalDataSourceConstraint` class to capture physical names of columns for retrieving availble interval
+
 - [CompositePhysicalTable Core Components Refactor](https://github.com/yahoo/fili/pull/179)
     * Added `ConcretePhysicalTable` and `ConcreteAvailability` to model table in druid datasource and its availabillity in the new table availability structure
     * Added class variable for `DataSourceMetadataService` and `ConfigurationLoader` into `AbstractBinderFactory` for application to access
@@ -59,6 +62,13 @@ Current
 
 ### Changed:
 
+- [Fix to use physical name instead of logical name to retrieve available interval](https://github.com/yahoo/fili/pull/226)
+    * `getAllAvailbleIntervals` in `ConcreteAvailability` no longer filters out unconfigured columns, instead table's `getAllAvailbleIntervals` does
+    * `getAvailbleIntervals` in `Availbality` now takes `PhysicalDataSourceConstraint` instead of `DataSourceConstraint`
+    * `Availbility` no longer takes a set of columns on the table, only table needs to know
+    * `getAllAvailbleIntervals` in `Availbility` now returns a map of column physical name to interval list instead of column to interval list
+    * `TestDataSourceMetadataService` now takes map from string to list of intervals instead of column to list of intervals for constructor
+
 - [Allow GranularityComparator to return static instance](https://github.com/yahoo/fili/pull/225)
     * Implementation of [PR #193](https://github.com/yahoo/fili/pull/193) suggests an possible improvement
     on GranularityComparator: Put the static instance on the GranularityComparator class itself, so that

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

@@ -41,10 +41,7 @@ public ConcretePhysicalTable(
                 timeGrain,
                 columns,
                 logicalToPhysicalColumnNames,
-                new ConcreteAvailability(
-                        name,
-                        metadataService
-                )
+                new ConcreteAvailability(name, metadataService)
         );
     }
 

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

@@ -44,10 +44,7 @@ public PermissiveConcretePhysicalTable(
                 timeGrain,
                 columns,
                 logicalToPhysicalColumnNames,
-                new PermissiveAvailability(
-                        name,
-                        dataSourceMetadataService
-                )
+                new PermissiveAvailability(name, dataSourceMetadataService)
         );
     }
 }

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

@@ -34,7 +34,7 @@ public interface Availability {
     /**
      * Fetch a set of intervals given a set of column name in DataSourceConstraint.
      *
-     * @param constraint  Data constraint containing columns and api filters
+     * @param constraint  Physical data source constraint containing column's physical name, metrics names, api filters
      *
      * @return A simplified list of intervals associated with all column in constraint, empty if column is missing
      */

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

@@ -51,7 +51,6 @@ public Set<TableName> getDataSourceNames() {
 
     @Override
     public Map<String, List<Interval>> getAllAvailableIntervals() {
-
         return metadataService.getAvailableIntervalsByTable(name);
     }
 
@@ -69,7 +68,7 @@ public SimplifiedIntervalList getAvailableIntervals(PhysicalDataSourceConstraint
         // Need to ensure requestColumns is not empty in order to prevent returning null by reduce operation
         return new SimplifiedIntervalList(
                 requestColumns.stream()
-                        .map(columnName -> allAvailableIntervals.getOrDefault(columnName, Collections.emptyList()))
+                        .map(physicalName -> allAvailableIntervals.getOrDefault(physicalName, Collections.emptyList()))
                         .map(intervals -> (Collection<Interval>) intervals)
                         .reduce(null, IntervalUtils::getOverlappingSubintervals)
         );

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

@@ -59,7 +59,7 @@ public DataSourceConstraint(DataApiRequest dataApiRequest, DruidAggregationQuery
      *
      * @param dataSourceConstraint  The data source constarint to copy from
      */
-    public DataSourceConstraint(DataSourceConstraint dataSourceConstraint) {
+    protected DataSourceConstraint(DataSourceConstraint dataSourceConstraint) {
         this.requestDimensions = dataSourceConstraint.getRequestDimensions();
         this.filterDimensions = dataSourceConstraint.getFilterDimensions();
         this.metricDimensions = dataSourceConstraint.getMetricDimensions();

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

@@ -58,7 +58,7 @@ class AvailabilityTestingUtils extends Specification {
                     Map<String, Set<Interval>> allIntervals = Stream.concat(
                             dimensionIntervals.entrySet().stream(),
                             metricIntervals.entrySet().stream()
-                    ).collect(Collectors.toMap({it.key}, { it.value }))
+                    ).collect(Collectors.toMap({ it.key }, { it.value }))
 
             // set new cache
                     table.setAvailability(new ConcreteAvailability(table.getTableName(), new TestDataSourceMetadataService(allIntervals)))

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

@@ -2,8 +2,6 @@ package com.yahoo.bard.webservice.table.availability
 
 import com.yahoo.bard.webservice.data.config.names.TableName
 import com.yahoo.bard.webservice.metadata.TestDataSourceMetadataService
-import com.yahoo.bard.webservice.table.Column
-import com.yahoo.bard.webservice.table.resolver.DataSourceConstraint
 import com.yahoo.bard.webservice.table.resolver.PhysicalDataSourceConstraint
 import com.yahoo.bard.webservice.util.SimplifiedIntervalList
 
@@ -17,34 +15,34 @@ import spock.lang.Specification
 class ConcreteAvailabilitySpec extends Specification{
 
     ConcreteAvailability concreteAvailability
-    String column1, column2, column3
+    String columnPhysicalName1, columnPhysicalName2, columnPhysicalName3
     Interval interval1, interval2
 
     def setup() {
 
-        column1 = 'column_one'
-        column2 = 'column_two'
-        column3 = 'column_three'
+        columnPhysicalName1 = 'column_one'
+        columnPhysicalName2 = 'column_two'
+        columnPhysicalName3 = 'column_three'
 
         interval1 = new Interval('2000-01-01/2015-12-31')
         interval2 = new Interval('2010-01-01/2020-12-31')
 
         concreteAvailability = new ConcreteAvailability(
                 TableName.of('table'),
                 new TestDataSourceMetadataService([
-                        (column1): [interval1] as Set,
-                        (column2): [interval2] as Set,
-                        'ignored_column': [interval1] as Set
+                        (columnPhysicalName1): [interval1] as Set,
+                        (columnPhysicalName2): [interval2] as Set,
+                        'hidden_column'     : [interval1] as Set
                 ])
         )
     }
 
-    def "getAllAvailability returns the correct availabilities for each column in datasource metadata service"() {
+    def "getAllAvailability returns all availabilities for all column in datasource metadata service"() {
         expect:
         concreteAvailability.getAllAvailableIntervals() == [
-                (column1): [interval1],
-                (column2): [interval2],
-                'ignored_column': [interval1],
+                (columnPhysicalName1): [interval1],
+                (columnPhysicalName2): [interval2],
+                'hidden_column'     : [interval1],
         ] as LinkedHashMap
     }
 
@@ -56,13 +54,13 @@ class ConcreteAvailabilitySpec extends Specification{
         concreteAvailability = new ConcreteAvailability(
                 TableName.of('table'),
                 new TestDataSourceMetadataService([
-                        (column1): [interval1] as Set,
-                        (column2): [interval2] as Set
+                        (columnPhysicalName1): [interval1] as Set,
+                        (columnPhysicalName2): [interval2] as Set
                 ])
         )
 
         PhysicalDataSourceConstraint dataSourceConstraint = Mock(PhysicalDataSourceConstraint)
-        dataSourceConstraint.getAllColumnPhysicalNames() >> [column1, column2]
+        dataSourceConstraint.getAllColumnPhysicalNames() >> [columnPhysicalName1, columnPhysicalName2]
 
         expect:
         concreteAvailability.getAvailableIntervals(dataSourceConstraint) == new SimplifiedIntervalList(
@@ -81,7 +79,7 @@ class ConcreteAvailabilitySpec extends Specification{
         '2017-01-01/2017-02-01' | '2017-01-15/2017-01-25' | ['2017-01-15/2017-01-25']  | "fully contain"
     }
 
-    def "getAvailableInterval returns empty interval if given column not configured to the table"() {
+    def "getAvailableInterval returns empty interval if given column is not in data source metadata service"() {
         given:
         PhysicalDataSourceConstraint constraint = Mock(PhysicalDataSourceConstraint)
         constraint.getAllColumnPhysicalNames() >> ['ignored']

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

@@ -0,0 +1,35 @@
+package com.yahoo.bard.webservice.table.resolver
+
+import com.yahoo.bard.webservice.data.time.ZonedTimeGrain
+import com.yahoo.bard.webservice.table.Column
+import com.yahoo.bard.webservice.table.PhysicalTableSchema
+
+import spock.lang.Specification
+
+/**
+ * Test PhysicalDataSourceConstraint correctly translates and filters column physical names.
+ */
+class PhysicalDataSourceConstraintSpec extends Specification {
+
+    DataSourceConstraint dataSourceConstraint
+    PhysicalTableSchema physicalTableSchema
+    PhysicalDataSourceConstraint physicalDataSourceConstraint
+
+    def setup() {
+        dataSourceConstraint = Mock(DataSourceConstraint)
+        dataSourceConstraint
+        dataSourceConstraint.getAllColumnNames() >> (['columnOne', 'columnTwo', 'columnThree'] as Set)
+        physicalTableSchema = new PhysicalTableSchema(Mock(ZonedTimeGrain), [new Column('columnOne'), new Column('columnThree')], ['columnOne': 'column_one', 'columnTwo': 'column_two'])
+        physicalDataSourceConstraint = new PhysicalDataSourceConstraint(dataSourceConstraint, physicalTableSchema)
+    }
+
+    def "physical data source constraint filters out columns not in the given schema"() {
+        expect:
+        !physicalDataSourceConstraint.getAllColumnPhysicalNames().contains('column_two')
+    }
+
+    def "physical data source constraint maps to physical name correctly even if mapping does not exist"() {
+        expect:
+        physicalDataSourceConstraint.getAllColumnPhysicalNames() == ['column_one', 'columnThree'] as Set
+    }
+}