Latest time macro #697
Latest time macro #697
Conversation
deepakb91
force-pushed
the
LatestTimeMacro
branch
from
12e022a
to
4682683
Compare
| physicalTableSchema.getPhysicalColumnName(_ as String) >> "" | ||
| physicalTableSchema.getColumns() >> Collections.emptySet() | ||
|
|
||
| SimplifiedIntervalList simplifiedIntervalList1 = Mock(SimplifiedIntervalList) |
There was a problem hiding this comment.
Instead of creating a Mock of SimplifiedIntervalList, should just create two new SimplifiedIntervalList
SimplifiedIntervalList simplifiedIntervalList1 = new SimplifiedIntervalList([interval1] as Set)
SimplifiedIntervalList simplifiedIntervalList2 = new SimplifiedIntervalList([interval2] as Set)
| ConfigPhysicalTable configPhysicalTable1 = Mock(ConfigPhysicalTable) | ||
| ConfigPhysicalTable configPhysicalTable2 = Mock(ConfigPhysicalTable) | ||
| configPhysicalTable1.getAvailability() >> availability1 | ||
| configPhysicalTable1.getAvailableIntervals() >> intervalList1 |
There was a problem hiding this comment.
Instead of getAvailableIntervals(), should use getAllAvailableIntervals() as it was used here
| @@ -145,6 +145,9 @@ Thanks to everyone who contributed to this release! | |||
|
|
|||
| ### Added: | |||
|
|
|||
| - [Logical Table Availability](https://github.com/yahoo/fili/pull/697) | |||
| * Added `logicalTableAvailability` to `TableUtils` which returns the union of availabilities for the logical table. | |||
|
|
|||
There was a problem hiding this comment.
This comment is incorrect.
| @@ -6,6 +6,8 @@ | |||
| * Feature flags bind an object to a system configuration name. | |||
| */ | |||
| public enum BardFeatureFlag implements FeatureFlag { | |||
|
|
|||
| CURRENT_MACRO_USES_LATEST("current_macro_uses_latest"), | |||
There was a problem hiding this comment.
Where's the comment for this PR?
|
The README comments on this PR were inaccurate and inadequate.
Michael McLawhorn
Senior Software Development Engineer
o: (217) 255-4215
On Wednesday, May 16, 2018, 5:37:25 PM CDT, deepakb91 <notifications@github.com> wrote:
Merged #697.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
| DateTime firstUnavailableInstant = TableUtils.logicalTableAvailability(getTable()).getLast().getEnd(); | ||
| DateTime adjustedNow = firstUnavailableInstant.isBeforeNow() ? firstUnavailableInstant : new DateTime(); | ||
|
|
||
| this.intervals = generateIntervals(adjustedNow, intervals, this.granularity, dateTimeFormatter); |
There was a problem hiding this comment.
Where's the test for this? And the documentation for what the feature flag is doing?
| DateTimeFormatter dateTimeFormatter = generateDateTimeFormatter(timeZone); | ||
|
|
||
| this.intervals = generateIntervals(intervals, this.granularity, dateTimeFormatter); | ||
| if (BardFeatureFlag.CURRENT_MACRO_USES_LATEST.isOn()) { |
There was a problem hiding this comment.
Why isn't this able to be handled by swapping the MacroCalculationStrategy for the CURRENT macro? There's already an extension point around macro behavior, so why doesn't it work for this use-case?
|
|
||
|
|
||
| /** | ||
| * Extracts the set of intervals from the api request. |
There was a problem hiding this comment.
How is this version of the method different from the one that doesn't take "now"? Why would the caller choose to use one over the other?
| DateTimeFormatter dateTimeFormatter = generateDateTimeFormatter(timeZone); | ||
|
|
||
| this.intervals = generateIntervals(intervals, this.granularity, dateTimeFormatter); | ||
| if (BardFeatureFlag.CURRENT_MACRO_USES_LATEST.isOn()) { | ||
| DateTime firstUnavailableInstant = TableUtils.logicalTableAvailability(getTable()).getLast().getEnd(); |
There was a problem hiding this comment.
How is this correct if it doesn't take in query constraints?
|
@cdeszaq https://github.com/yahoo/fili/pull/702/files is a partial remedy to this PR, but it certainly deserves a larger discussion. Unfortunately, when that yahoo-oath transition happened, all of the fili core team members who had access to the https://groups.google.com/forum/#!forum/fili-users and https://groups.google.com/forum/#!forum/fili-developers lost their ability to connect. We haven't really established a forum for public discussions outside of issues and I think this needs more of a retro discussion than an issue. Do you still have admin access to those groups such that you could open them up a little wider? |
|
@michael-mclawhorn Yes, a bit broader discussion would be good. Time is hard, and a lot of deep thought, sweat, blood, and tears when into Fili's treatment of it, so I would hate for that to be lost or made less reliable. As for those groups: There's also the Core Team group, which is visible to members-only and does require approval when requesting to join, but no one has requested. As folks join the groups with any new email addresses[1], I'll give them the same permissions they had before if I recognize who their email likely ties to, so we should be able to re-constitute the group membership. (I've already given your 2nd account ownership privileges, so you should be able to manage whichever forum that was). Once everyone who cares is re-populated, we can clean up accounts and names so everyone doesn't have a 2 after their name ;-) There's also Gitter if more real-time conversation is useful, though for something like time and macros which can be quite complex, something a bit more long-form is probably useful. [1]: I recommend using personal accounts for this reason... work contacts are efemeral |
No description provided.