Skip to content

Commit

Permalink
Modified according to rick and mike's comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@garyluoex
garyluoex committed Apr 17, 2017
1 parent f8623ba commit 8f4da9a
Show file tree
Hide file tree
Showing 5 changed files with 151 additions and 108 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ Current
* `PhysicalTableDefinition` now requires a `build` methods to be implemented that builds a physical table
* `BaseTableLoader` now constructs physical tables by calling `build` method on `PhysicalTableDefinition`s in `buildPhysicalTablesWithDependency`
* `buildDimensionSpanningTableGroup` method in `BaseTableLoader` now uses `loadPhysicalTablesWithDependency` instead of removed `loadPhysicalTables`
* `buildDimensionSpanningTableGroup` method in `BaseTableLoader` now does not take druid metric as arguments, instead `PhysicalTableDefinition` does
* `buildDimensionSpanningTableGroup` method in `BaseTableLoader` now does not take druid metrics as arguments, instead `PhysicalTableDefinition` does

- [Make `TemplateDruidQuery::getMetricField` get the first field instead of any field](https://github.com/yahoo/fili/pull/210)
* Previously, order was by luck, now it's by the contract of `findFirst`
Expand Down
115 changes: 59 additions & 56 deletions fili-core/src/main/java/com/yahoo/bard/webservice/data/config/table/BaseTableLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -71,39 +70,45 @@ protected BaseTableLoader(DateTimeZone defaultTimeZone, DataSourceMetadataServic
public abstract void loadTableDictionary(ResourceDictionaries dictionaries);

/**
* Builds a table group.
* Builds a table group that derive its dimensions by taking the union of all the underlying physical dimensions.
* <p>
* Builds and loads the physical tables from table definitions for the current table group.
*
* @param tableNames Set of table name of tables belonging to this table group
* @param currentTableGroupTableNames Set of table name of tables belonging to this table group
* @param tableDefinitions A list of config objects for building physical tables and its dependent physical tables
* @param dictionaries The container for all the data dictionaries
* @param apiMetrics The set of metric names surfaced to the api
*
* @return A table group binding all the tables for a logical table view together.
*/
public TableGroup buildDimensionSpanningTableGroup(
Set<TableName> tableNames,
Set<TableName> currentTableGroupTableNames,
Set<PhysicalTableDefinition> tableDefinitions,
ResourceDictionaries dictionaries,
Set<ApiMetricName> apiMetrics
) {
Map<String, PhysicalTableDefinition> availableTableDefinitions =
buildPhysicalTableDefinitionDictionary(tableDefinitions);
Map<String, PhysicalTableDefinition> availableTableDefinitions = buildPhysicalTableDefinitionDictionary(
tableDefinitions
);

PhysicalTableDictionary physicalTableDictionary = dictionaries.getPhysicalDictionary();

// Get the physical table from physical table dictionary, if not exist, build it and put it in dictionary
LinkedHashSet<PhysicalTable> physicalTables = tableNames.stream()
LinkedHashSet<PhysicalTable> physicalTables = currentTableGroupTableNames.stream()
.map(TableName::asName)
.map(
tableName -> physicalTableDictionary.computeIfAbsent(
tableName.asName(),
key -> buildPhysicalTablesWithDependency(key, availableTableDefinitions, dictionaries)
tableName,
tableNameKey -> buildPhysicalTableWithDependency(
tableNameKey,
availableTableDefinitions,
dictionaries
)
)
).collect(Collectors.toCollection(LinkedHashSet::new));


//Derive the logical dimensions by taking the union of all the physical dimensions
// Derive the dimensions by taking the union of all the physical dimensions
Set<Dimension> dimensions = physicalTables.stream()
.map(PhysicalTable::getSchema)
.map(schema -> schema.getColumns(DimensionColumn.class))
Expand Down Expand Up @@ -167,15 +172,18 @@ public void loadLogicalTableWithGranularities(
}

/**
* Iterate through given definitions and builds all corresponding physical table with satisfied dependency.
* Build and return the current physical table given its table name and definition, if dependency exists, build its
* dependencies and load the dependencies into physical table dictionary.
* <p>
* note: current physical table not loaded into physical table dictionary, only dependencies will
*
* @param currentTableName Iterator for the mutable definition
* @param availableTableDefinitions A map of table name to table definition that are available globally
* @param dictionaries Contains both dimension and physical table dictionary for building and dependency resolution
*
* @return the current physical table built from table definitions
*/
protected PhysicalTable buildPhysicalTablesWithDependency(
protected PhysicalTable buildPhysicalTableWithDependency(
String currentTableName,
Map<String, PhysicalTableDefinition> availableTableDefinitions,
ResourceDictionaries dictionaries
Expand All @@ -197,36 +205,19 @@ protected PhysicalTable buildPhysicalTablesWithDependency(
"circular dependency", currentTableName);

LOG.error(message);
throw new RuntimeException(message);
throw new IllegalStateException(message);
}

// If dependent table not in physical table dictionary, try to recursively build it and put it in the dictionary
currentTableDefinition.getDependentTableNames()
.forEach(tableName -> physicalTableDictionary.computeIfAbsent(
tableName.asName(),
name -> buildPhysicalTablesWithDependency(
name,
availableTableDefinitions,
dictionaries
)
name -> buildPhysicalTableWithDependency(name, availableTableDefinitions, dictionaries)
));

LOG.debug("Table Loader Building Physical Table {}");
// Build the current physical table using physical table dictionary to resolve dependency
Optional<PhysicalTable> optionalPhysicalTable = currentTableDefinition.build(
dictionaries,
getDataSourceMetadataService()
);

// If the table definition correctly builds a physical table
if (optionalPhysicalTable.isPresent()) {
return optionalPhysicalTable.get();
} else {
String message = String.format(
"Unable to build physical table: %s, might be table definition error", currentTableName);

LOG.error(message);
throw new RuntimeException(message);
}
return currentTableDefinition.build(dictionaries, getDataSourceMetadataService());
}

/**
Expand All @@ -252,6 +243,31 @@ private Map<String, PhysicalTableDefinition> buildPhysicalTableDefinitionDiction
.collect(Collectors.toMap(definition -> definition.getName().asName(), Function.identity()));
}

/**
* Load a new physical table into the dictionary and return the loaded physical table.
*
* @param definition A config object for the physical table
* @param metricNames The Set of metric names on the table
* @param dictionaries The resource dictionaries for reading and storing resource data
*
* @return The physical table created
*
* @deprecated use buildPhysicalTableWithDependency instead, which supports building table with dependencies
*/
@Deprecated
protected PhysicalTable loadPhysicalTable(
PhysicalTableDefinition definition,
Set<FieldName> metricNames,
ResourceDictionaries dictionaries
) {
LOG.debug(
"Building table {} with deprecated loadPhysicalTable method, use buildPhysicalTableWithDependency " +
"instead",
definition.getName().asName()
);
return definition.build(dictionaries, getDataSourceMetadataService());
}

/**
* Builds a table group.
* <p>
Expand All @@ -266,36 +282,23 @@ private Map<String, PhysicalTableDefinition> buildPhysicalTableDefinitionDiction
*
* @deprecated does not load table with external dependency, use the other buildDimensionSpanningTableGroup instead
*/
@Deprecated
public TableGroup buildDimensionSpanningTableGroup(
Set<ApiMetricName> apiMetrics,
Set<FieldName> druidMetrics,
Set<PhysicalTableDefinition> tableDefinitions,
ResourceDictionaries dictionaries
) {
Map<String, PhysicalTableDefinition> availableTableDefinitions =
buildPhysicalTableDefinitionDictionary(tableDefinitions);

PhysicalTableDictionary physicalTableDictionary = dictionaries.getPhysicalDictionary();

// Get the physical table from physical table dictionary, if not exist, build it and put it in dictionary
LinkedHashSet<PhysicalTable> physicalTables = tableDefinitions.stream()
.map(
tableName -> physicalTableDictionary.computeIfAbsent(
tableName.getName().asName(),
key -> buildPhysicalTablesWithDependency(key, availableTableDefinitions, dictionaries)
)
).collect(Collectors.toCollection(LinkedHashSet::new));

Set<TableName> currentTableGroupTableNames = tableDefinitions.stream()
.map(PhysicalTableDefinition::getName)
.collect(Collectors.toSet());

//Derive the logical dimensions by taking the union of all the physical dimensions
Set<Dimension> dimensions = physicalTables.stream()
.map(PhysicalTable::getSchema)
.map(schema -> schema.getColumns(DimensionColumn.class))
.flatMap(Set::stream)
.map(DimensionColumn::getDimension)
.collect(Collectors.toCollection(LinkedHashSet::new));

return new TableGroup(physicalTables, apiMetrics, dimensions);
return buildDimensionSpanningTableGroup(
currentTableGroupTableNames,
tableDefinitions,
dictionaries,
apiMetrics
);
}

/**
Expand Down
33 changes: 24 additions & 9 deletions ...ain/java/com/yahoo/bard/webservice/data/config/table/ConcretePhysicalTableDefinition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import com.yahoo.bard.webservice.table.PhysicalTable;

import java.util.Collections;
import java.util.Optional;
import java.util.Map;
import java.util.Set;

/**
Expand All @@ -37,24 +37,39 @@ public ConcretePhysicalTableDefinition(
super(name, timeGrain, metricNames, dimensionConfigs);
}

/**
* Define a physical table with provided logical to physical column name mappings.
*
* @param name The table name
* @param timeGrain The zoned time grain
* @param metricNames The Set of metric names on the table
* @param dimensionConfigs The dimension configurations
* @param logicalToPhysicalNames A map from logical column names to physical column names
*/
public ConcretePhysicalTableDefinition(
TableName name,
ZonedTimeGrain timeGrain,
Set<FieldName> metricNames,
Set<? extends DimensionConfig> dimensionConfigs,
Map<String, String> logicalToPhysicalNames

) {
super(name, timeGrain, metricNames, dimensionConfigs, logicalToPhysicalNames);
}

@Override
public Set<TableName> getDependentTableNames() {
return Collections.emptySet();
}

@Override
public Optional<PhysicalTable> build(
ResourceDictionaries dictionaries,
DataSourceMetadataService metadataService
) {
return Optional.of(
new ConcretePhysicalTable(
public PhysicalTable build(ResourceDictionaries dictionaries, DataSourceMetadataService metadataService) {
return new ConcretePhysicalTable(
getName(),
getTimeGrain(),
buildColumns(dictionaries.getDimensionDictionary()),
getLogicalToPhysicalNames(),
metadataService
)
);
);
}
}
38 changes: 33 additions & 5 deletions ...re/src/main/java/com/yahoo/bard/webservice/data/config/table/PhysicalTableDefinition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,15 @@
import com.yahoo.bard.webservice.table.Column;
import com.yahoo.bard.webservice.table.PhysicalTable;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand All @@ -28,6 +31,7 @@
* Common configurations necessary to build a physical table.
*/
public abstract class PhysicalTableDefinition {
private static final Logger LOG = LoggerFactory.getLogger(PhysicalTableDefinition.class);

private final TableName name;
private final ZonedTimeGrain timeGrain;
Expand Down Expand Up @@ -56,6 +60,29 @@ protected PhysicalTableDefinition(
this.logicalToPhysicalNames = Collections.unmodifiableMap(buildLogicalToPhysicalNames(dimensionConfigs));
}

/**
* Constructor with provided logical to physical name mapping.
*
* @param name Table name of the physical table
* @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 logicalToPhysicalNames A map from logical column names to physical column names
*/
protected PhysicalTableDefinition(
TableName name,
ZonedTimeGrain timeGrain,
Set<FieldName> metricNames,
Set<? extends DimensionConfig> dimensionConfigs,
Map<String, String> logicalToPhysicalNames
) {
this.name = name;
this.timeGrain = timeGrain;
this.metricNames = ImmutableSet.copyOf(metricNames);
this.dimensionConfigs = ImmutableSet.copyOf(dimensionConfigs);
this.logicalToPhysicalNames = ImmutableMap.copyOf(logicalToPhysicalNames);
}

/**
* Get the set of physical tables required to build the current physical table.
*
Expand All @@ -64,14 +91,14 @@ protected PhysicalTableDefinition(
public abstract Set<TableName> getDependentTableNames();

/**
* Given a set of metric names and a data source metadata service, build the corresponding physical table.
* Given the resource dictionaries and a data source metadata service, build the corresponding physical table.
*
* @param dictionaries Dictionary containing dimension dictionary and physical table dictionary
* @param metadataService Service containing column available interval information
*
* @return optional of the corresponding type of physical table, empty optional if could not be build.
*/
public abstract Optional<PhysicalTable> build(
public abstract PhysicalTable build(
ResourceDictionaries dictionaries,
DataSourceMetadataService metadataService
);
Expand Down Expand Up @@ -111,8 +138,9 @@ protected Map<String, String> buildLogicalToPhysicalNames(Set<? extends Dimensio
config -> {
String physicalName = config.getPhysicalName();
if (physicalName == null) {
throw new RuntimeException("No physical name found for dimension: "
+ config.getApiName());
LOG.debug("No physical name found for dimension: "
+ config.getApiName(), "using api name as physical name");
return config.getApiName();
}
return physicalName;
}
Expand Down
Loading

0 comments on commit 8f4da9a

Please sign in to comment.