Remove restriction for single physical dimension to multiple lookup dimension #112

garyluoex · 2016-11-29T19:50:13Z

Modified physical dimension name to logical dimension name mapping into a Map<String, List<String>> instead of Map<String, String> in PhysicalTable.java

…lookup dimension

archolewa · 2016-11-29T20:10:53Z

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

@@ -38,7 +40,7 @@
    private Map<Column, Set<Interval>> workingIntervals;
    private final Object mutex = new Object();
    private final Map<String, String> logicalToPhysicalColumnNames;
-    private final Map<String, String> physicalToLogicalColumnNames;
+    private final Map<String, List<String>> physicalToLogicalColumnNames;


Any particular reason we're making this a List rather than a Collection?

Does Set fit better (since it should be a unique set of logical dimensions)? I'm not sure what are the advantages of just using Collection, fill me in if you have time, thanks!

It's really more about avoiding overspecification. The places we use it only needs a Collection (we're iterating over every element). And at least in the current implementation we don't seem overly concerned with uniqueness. Though having the same dimension twice could lead to weirdness if addNewDimensionColumn isn't idempotent.

I'm not too concerned about it, it's just that we usually default to Collection unless we have a good reason for using a more specific type.

I don't think we want to have any headaches around nested types. Let's just use set for now.

Ok, will make it Set since it makes sense to add a uniqueness constraint and doesn't make sense that there are duplicates.

archolewa · 2016-11-29T20:14:16Z

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

+                        continue;
+                    }
+                    DimensionColumn dimensionColumn = DimensionColumn.addNewDimensionColumn(this, dimension);
+                    workingIntervals.put(dimensionColumn, nameIntervals.getValue());


You can streamify this easily enough, I think:

getLogicalColumnName(physicalName).stream() .map(dimensionDictionary::findByApiName) .filter(dimension -> dimension != null) .forEach(dimension -> { DimensionColumn dimensionColumn = DimensionColumn.addNewDimensionColumn(this, dimension); workingIntervals.put(dimensionColumn, nameIntervals.getValue()); });

Note that you could put DimensionColumn.addNewDimensionColumn(this, dimension) in a map rather than the first statement of a forEach, but addNewDimensionColumn has side-effects, and side-effecting operations in a map operation feels ugly to me.

archolewa · 2016-11-29T20:14:48Z

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

@@ -237,8 +244,8 @@ public String getPhysicalColumnName(String logicalName) {
     * @param physicalName  Physical name to lookup in physical table
     * @return Translated physicalName if applicable
     */
-    private String getLogicalColumnName(String physicalName) {
-        return physicalToLogicalColumnNames.getOrDefault(physicalName, physicalName);
+    private List<String> getLogicalColumnName(String physicalName) {


Again, this could probably be a Collection rather than a List.

garyluoex · 2016-11-29T20:19:53Z

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

-                        .entrySet()
-                        .stream()
-                        .collect(StreamUtils.toLinkedMap(Map.Entry::getValue, Map.Entry::getKey))
+                this.logicalToPhysicalColumnNames.entrySet().stream().sequential().collect(


I think removing the sequential() here is fine right? Will remove it in the next commit if no one complains.

I believe sequential just forces the stream to be sequential, even if it was originally parallel. However, by default the stream of a set is sequential, so sequential doesn't actually do anything.

So yes, I would suggest removing the sequential.

michael-mclawhorn · 2016-11-29T20:51:57Z

The fact that this didn't break a test worries me. Please add a test to see that the correct set of logical column names is returned from the method in both the single-dimension-to-physical-name situation and the multiple-dimension-to-physical-name situation.

michael-mclawhorn · 2016-11-29T20:48:09Z

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

-    private List<String> getLogicalColumnName(String physicalName) {
-        return physicalToLogicalColumnNames.getOrDefault(physicalName, Arrays.asList(physicalName));
+    private Set<String> getLogicalColumnName(String physicalName) {
+        return physicalToLogicalColumnNames.getOrDefault(physicalName, new HashSet<>(Arrays.asList(physicalName)));
    }


Pluralize this method name. getLogicalColumnNames

michael-mclawhorn · 2016-11-29T20:48:30Z

CHANGELOG.md

@@ -14,6 +14,9 @@ Current

 ### Changed:

+- [Remove restriction for single physical dimension to multiple lookup dimensions](https://github.com/yahoo/fili/pull/112)
+    * Modified physical dimension name to logical dimension name mapping into a `Map<String, List<String>>` instead of `Map<String, String>` in `PhysicalTable.java`
+


Keep in synch with the code change.

garyluoex · 2016-11-29T20:56:57Z

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

-                workingIntervals.put(dimensionColumn, nameIntervals.getValue());
+
+                getLogicalColumnName(physicalName).stream()
+                        .sequential()


It seems like this is extra again, will remove in the next commit again.

cdeszaq

Nothing big, just a few trivial cleanups. 👍

cdeszaq · 2016-11-29T23:50:08Z

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

+
+                getLogicalColumnNames(physicalName).stream()
+                        .map(dimensionDictionary::findByApiName)
+                        .filter(dimension -> dimension != null)


Objects::isNotNull simplifies this I think

cdeszaq · 2016-11-29T23:51:24Z

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

+                        .filter(dimension -> dimension != null)
+                        .forEach(
+                                dimension -> {
+                                    DimensionColumn dimensionColumn = DimensionColumn.addNewDimensionColumn(


This can be a .map() call on the stream, since you're just transforming the Dimension into a DimensionColumn

But the operations will cause side-affects right (adding things to existing things)? so if the stream runs in parallel things will get synchronization problem. forEach is a reduce operation that allows side-affects I believe. Let me know if I'm wrong or not making sense, thanks!

I'm with @garyluoex on this one. It's not a big deal, especially since we don't run around randomly making streams parallel (and we shouldn't, not without carefully examining the operations in the stream first), but it just feels deeply and fundamentally wrong to me to put a side-effectful operation in a map.

To elaborate on why it feels wrong to me, I generally prefer to be hit across the head with a big neon sign when an operation has side-effects, especially when the operation returns a value as well. forEach does that, while a map operation does not.

archolewa · 2016-11-30T16:17:31Z

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

+    def "test physical to logical mapping is constructed correctly"() {
+        setup:
+        def oneDimPhysicalTable = new PhysicalTable("test table", DAY.buildZonedTimeGrain(UTC), ['dimension':'druidDim'])
+        def twoDimPhysicalTable = new PhysicalTable("test table", DAY.buildZonedTimeGrain(UTC), ['dimension1':'druidDim', 'dimension2':'druidDim'])


Unless we have good reason to be taking advantage of Groovy's duck-typing, we should be explicitly specifying the types of the variables.

Just to make things shorter and simpler to read since the type should be obvious from the assignment initialization in this case (it feels like it is more intuitive to me), let me know if there are any consequences of doing this or it is just good practice to declare type?

At least in the context of our project, it's considered good practice (I don't know enough about the larger Groovy community to know if that's true more generally).

If nothing else, it improves IDE support, because the IDE knows what the variable type is.

It can also make things more readable, because it's easy for readers to see what the variable type this (though this can be partially offset because def is more concise, so which is more readable is going to be a matter of opinion).

Duck typing has its advantages, but it has its disadvantages too. Here we're not using any of the advantages, but we're still saddled with the disadvantages.

ok will change it then, thanks!

archolewa · 2016-11-30T16:19:08Z

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

-    private String getLogicalColumnName(String physicalName) {
-        return physicalToLogicalColumnNames.getOrDefault(physicalName, physicalName);
+    private Set<String> getLogicalColumnNames(String physicalName) {
+        return physicalToLogicalColumnNames.getOrDefault(physicalName, new HashSet<>(Arrays.asList(physicalName)));


Should use Collections.singleton.

archolewa · 2016-11-30T16:19:48Z

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

+                        .map(dimensionDictionary::findByApiName)
+                        .filter(Objects::nonNull)
+                        .forEach(
+                        dimension -> {


The indentation seems off here. dimension -> should be indented eight spaces, and everything inside the lambda should be indented four spaces past that.

archolewa · 2016-11-30T16:21:04Z

Had a few small comments, 👍 once they've been addressed.

garyluoex changed the title ~~Remove restriction for single physcial dimension to multiple lookup dimension~~ Remove restriction for single physical dimension to multiple lookup dimension Nov 29, 2016

garyluoex added NEED 2 REVIEWS REVIEWABLE labels Nov 29, 2016

garyluoex force-pushed the ManyPhysical branch from 9ef0e67 to 9084e10 Compare November 29, 2016 20:06

Remove restriction for single physcial dimension mapping to multiple …

a8097e7

…lookup dimension

garyluoex force-pushed the ManyPhysical branch from 9084e10 to a8097e7 Compare November 29, 2016 20:10

archolewa requested changes Nov 29, 2016

View reviewed changes

archolewa added the NEED CHANGES label Nov 29, 2016

garyluoex commented Nov 29, 2016

View reviewed changes

Modified according to andrew's comment

9d5fe53

michael-mclawhorn reviewed Nov 29, 2016

View reviewed changes

garyluoex commented Nov 29, 2016

View reviewed changes

Modified acording to mike's comments

e0782bf

garyluoex removed the NEED CHANGES label Nov 29, 2016

cdeszaq approved these changes Nov 30, 2016

View reviewed changes

Modified according to rick's comments

dc2e3a8

garyluoex added NEED 1 REVIEW and removed NEED 2 REVIEWS labels Nov 30, 2016

archolewa requested changes Nov 30, 2016

View reviewed changes

archolewa added NEED CHANGES and removed NEED 1 REVIEW REVIEWABLE labels Nov 30, 2016

Modified according to andrew's second round comments

eaffeca

garyluoex added MERGEABLE and removed NEED CHANGES labels Nov 30, 2016

archolewa approved these changes Nov 30, 2016

View reviewed changes

archolewa merged commit 35b1557 into master Nov 30, 2016

archolewa deleted the ManyPhysical branch November 30, 2016 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove restriction for single physical dimension to multiple lookup dimension #112

Remove restriction for single physical dimension to multiple lookup dimension #112

garyluoex commented Nov 29, 2016 •

edited

Loading

archolewa Nov 29, 2016

garyluoex Nov 29, 2016

archolewa Nov 29, 2016 •

edited

Loading

michael-mclawhorn Nov 29, 2016

garyluoex Nov 29, 2016

archolewa Nov 29, 2016

archolewa Nov 29, 2016

garyluoex Nov 29, 2016

archolewa Nov 29, 2016

michael-mclawhorn commented Nov 29, 2016

michael-mclawhorn Nov 29, 2016

michael-mclawhorn Nov 29, 2016

garyluoex Nov 29, 2016

cdeszaq left a comment

cdeszaq Nov 29, 2016

cdeszaq Nov 29, 2016

garyluoex Nov 30, 2016

archolewa Nov 30, 2016

archolewa Nov 30, 2016

archolewa Nov 30, 2016

garyluoex Nov 30, 2016 •

edited

Loading

archolewa Nov 30, 2016

garyluoex Nov 30, 2016

archolewa Nov 30, 2016

archolewa Nov 30, 2016

archolewa commented Nov 30, 2016

Remove restriction for single physical dimension to multiple lookup dimension #112

Remove restriction for single physical dimension to multiple lookup dimension #112

Conversation

garyluoex commented Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archolewa Nov 29, 2016 • 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

michael-mclawhorn commented Nov 29, 2016

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

garyluoex Nov 30, 2016 • 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

archolewa commented Nov 30, 2016

garyluoex commented Nov 29, 2016 •

edited

Loading

archolewa Nov 29, 2016 •

edited

Loading

garyluoex Nov 30, 2016 •

edited

Loading