Conversation
Uses constants in place of literal strings for context keys. Moves some QueryContext methods to QueryContexts for reuse.
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
Generally looks good. Just wanted to sort out the differences between parse stuff and getAs stuff.
| public static <T> int getUncoveredIntervalsLimit(Query<T> query, int defaultValue) | ||
| { | ||
| return parseInt(query, "uncoveredIntervalsLimit", defaultValue); | ||
| return parseInt(query, UNCOVERED_INTERVALS_LIMIT_KEY, defaultValue); |
There was a problem hiding this comment.
Is there any reason for parseInt and getAsInt to remain different? Now that we have getAsInt, shall it replace parseInt?
(Btw, is the behavior the same? I haven't checked)
| public static <T> boolean isSerializeDateTimeAsLongInner(Query<T> query, boolean defaultValue) | ||
| { | ||
| return parseBoolean(query, "serializeDateTimeAsLongInner", defaultValue); | ||
| return parseBoolean(query, SERIALIZE_DATE_TIME_AS_LONG_INNER_KEY, defaultValue); |
There was a problem hiding this comment.
Is there any reason for parseBoolean and getAsBoolean to remain different? Now that we have getAsBoolean, shall it replace parseBoolean?
(Btw, is the behavior the same? I haven't checked)
There was a problem hiding this comment.
Good point. I didn't really pay attention to the parseFoo methods. Those methods appear to only handle the value-as-string case, where as the getAsFoo method handle maps with values of the expected type. Went ahead and changed the parseFoo methods to use getAsFoo so we're consistent.
3 participants
Query context cleanup in support of the planner test framework PR. This PR splits up the files from the planner test PR to make review a bit easier.
QueryContextconstants in place of literal string keys for several commonly-used keys.QueryContexttoQueryContextsso that they can be reused, such as in the planner tests.No functional changes; only refactoring.
This PR has: