-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Latest time macro #697
Latest time macro #697
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where's the comment for this PR? |
||
PARTIAL_DATA("partial_data_enabled"), | ||
@Deprecated DRUID_CACHE("druid_cache_enabled"), | ||
@Deprecated DRUID_CACHE_V2("druid_cache_v2_enabled"), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,7 @@ | |
import com.yahoo.bard.webservice.web.ErrorMessageFormat; | ||
import com.yahoo.bard.webservice.web.FilterOperation; | ||
import com.yahoo.bard.webservice.web.ResponseFormatType; | ||
import com.yahoo.bard.webservice.web.TimeMacros; | ||
import com.yahoo.bard.webservice.web.TimeMacro; | ||
import com.yahoo.bard.webservice.web.filters.ApiFilters; | ||
import com.yahoo.bard.webservice.web.util.PaginationLink; | ||
import com.yahoo.bard.webservice.web.util.PaginationParameters; | ||
|
@@ -385,6 +385,27 @@ protected static Set<Interval> generateIntervals( | |
String apiIntervalQuery, | ||
Granularity granularity, | ||
DateTimeFormatter dateTimeFormatter | ||
) throws BadApiRequestException { | ||
return generateIntervals(new DateTime(), apiIntervalQuery, granularity, dateTimeFormatter); | ||
} | ||
|
||
|
||
/** | ||
* Extracts the set of intervals from the api request. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
* | ||
* @param now The 'now' for which time macros will be relatively calculated | ||
* @param apiIntervalQuery API string containing the intervals in ISO 8601 format, values separated by ','. | ||
* @param granularity The granularity to generate the date based on period or macros. | ||
* @param dateTimeFormatter The formatter to parse date time interval segments | ||
* | ||
* @return Set of jodatime interval objects. | ||
* @throws BadApiRequestException if the requested interval is not found. | ||
*/ | ||
protected static Set<Interval> generateIntervals( | ||
DateTime now, | ||
String apiIntervalQuery, | ||
Granularity granularity, | ||
DateTimeFormatter dateTimeFormatter | ||
) throws BadApiRequestException { | ||
try (TimedPhase timer = RequestLog.startTiming("GeneratingIntervals")) { | ||
Set<Interval> generated = new LinkedHashSet<>(); | ||
|
@@ -417,7 +438,6 @@ protected static Set<Interval> generateIntervals( | |
} | ||
|
||
Interval interval; | ||
DateTime now = new DateTime(); | ||
//If start interval is period, then create new interval with computed end date | ||
//possible end interval could be next,current, date | ||
if (start.startsWith("P")) { | ||
|
@@ -458,7 +478,6 @@ protected static Set<Interval> generateIntervals( | |
return generated; | ||
} | ||
} | ||
|
||
/** | ||
* Generates filter objects on the based on the filter query in the api request. | ||
* | ||
|
@@ -550,7 +569,7 @@ public static DateTime getAsDateTime( | |
DateTimeFormatter timeFormatter | ||
) throws BadApiRequestException { | ||
//If granularity is all and dateText is macro, then throw an exception | ||
TimeMacros macro = TimeMacros.forName(dateText); | ||
TimeMacro macro = TimeMacro.forName(dateText); | ||
if (macro != null) { | ||
if (granularity instanceof AllGranularity) { | ||
LOG.debug(INVALID_INTERVAL_GRANULARITY.logFormat(macro, dateText)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,7 @@ | |
import com.yahoo.bard.webservice.table.LogicalTableDictionary; | ||
import com.yahoo.bard.webservice.table.TableIdentifier; | ||
import com.yahoo.bard.webservice.util.StreamUtils; | ||
import com.yahoo.bard.webservice.util.TableUtils; | ||
import com.yahoo.bard.webservice.web.ApiFilter; | ||
import com.yahoo.bard.webservice.web.ApiHaving; | ||
import com.yahoo.bard.webservice.web.BadApiRequestException; | ||
|
@@ -294,10 +295,16 @@ public DataApiRequestImpl( | |
LOG.debug(TABLE_UNDEFINED.logFormat(tableName)); | ||
throw new BadApiRequestException(TABLE_UNDEFINED.format(tableName)); | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why isn't this able to be handled by swapping the |
||
DateTime firstUnavailableInstant = TableUtils.logicalTableAvailability(getTable()).getLast().getEnd(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is this correct if it doesn't take in query constraints? |
||
DateTime adjustedNow = firstUnavailableInstant.isBeforeNow() ? firstUnavailableInstant : new DateTime(); | ||
|
||
this.intervals = generateIntervals(adjustedNow, intervals, this.granularity, dateTimeFormatter); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where's the test for this? And the documentation for what the feature flag is doing? |
||
} else { | ||
this.intervals = generateIntervals(intervals, this.granularity, dateTimeFormatter); | ||
} | ||
|
||
this.filterBuilder = druidFilterBuilder; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is incorrect.