add a sql option to force user to specify time condition #6246
Conversation
|
I quite like this feature |
|
Hi @gianm , could you take a look at this? |
|
@QiuMM Can you fix conflicts? |
|
conflict resolved. @jon-wei could you help review? |
| @@ -57,6 +57,9 @@ | |||
| @JsonProperty | |||
| private boolean useFallback = false; | |||
|
|
|||
| @JsonProperty | |||
| private boolean forceTimeCondition = false; | |||
There was a problem hiding this comment.
Suggest "requireTimeCondition" as the name instead.
"force" to me sounds like requiring the server to plan queries in a certain way, while this is more of a requirement on queries submitted by the user.
| final Query innerMostQuery = findInnerMostQuery(query); | ||
| if (plannerContext.getPlannerConfig().isForceTimeCondition() && | ||
| innerMostQuery.getIntervals().equals(Intervals.ONLY_ETERNITY)) { | ||
| throw new CannotBuildQueryException("Missing __time filter"); |
There was a problem hiding this comment.
Suggest having this error message mention the time condition property, so that users who encounter it know that the problem is with their query and not an internal server error.
e.g. "requireTimeCondition is enabled, all queries must include a filter condition on the __time column."
|
@jon-wei thanks for your review, all comments have been addressed. |
One downside of SQL's flexibility is that users tend to forget adding time filter when writing queries, which is usually very bad for timeseries data analysis and causes stability issue.
Since native query requires users to specify "intervals", I think it's reasonable to force SQL users to specify __time filter in many cases.
This PR adds a new option
druid.sql.planner.requireTimeConditionto support such needs. It defaults to false though so that the default behaviour is the same as before.