Catalog granularity accepts query format#16680
Conversation
| granularity = new PeriodGranularity(new Period(value), null, null); | ||
| } | ||
| catch (IllegalArgumentException e2) { | ||
| throw new IAE(StringUtils.format("[%s] is an invalid granularity string", value)); |
There was a problem hiding this comment.
DruidException instead of IAE:
| throw new IAE(StringUtils.format("[%s] is an invalid granularity string", value)); | |
| throw InvalidInput.exception("[%s] is an invalid granularity string.", value); |
There was a problem hiding this comment.
This caused a test failure. i see that there are catch blocks in places expecting IAE, dont want to fuss with that at the moment. I think we can change all of these throws in a different pr. ok with you?
There was a problem hiding this comment.
Yep, sounds good to me. I noticed that we throw IAE in a bunch of places in CatalogUtils which can be addressed in a separate PR.
| @@ -68,12 +68,20 @@ public static Granularity asDruidGranularity(String value) | |||
| if (Strings.isNullOrEmpty(value) || value.equalsIgnoreCase(DatasourceDefn.ALL_GRANULARITY)) { | |||
There was a problem hiding this comment.
Are there tests for this static utility or perhaps something more end-to-end? If so, it'd be nice if we could add some tests with granularity strings like HOUR and DAY for the enhancement
There was a problem hiding this comment.
updated existing integration tests to test all formats P1D, DAY, ALL
There was a problem hiding this comment.
LGTM! It seems that the UT/ITs haven't run on this PR, and the catalog integration tests haven't run either?
(Edit: Never mind, it seems that the UT/IT checks are still pending: https://github.com/apache/druid/actions/runs/9753473200. I got tripped up by the blue check with only 12 successful checks, all the checks should eventually appear)
3 participants
Description
Previously, the segment granularity for tables in the catalog had to be defined in period format, ie
'PT1H','P1D', etc. This disallows a user from defining segment granularity of'ALL'for a table in the catalog, which may be a valid use case. This change makes it so that a user may define the segment granularity of a table in the catalog, as any string that results in a valid granularity using either theGranularity.fromString(str)method, ornew PeriodGranularity(new Period(value), null, null), and that granularity maps to a standard supported granularity, whereGranularityType.isStandard(granularity)returns true. As a result a user may who wants to assign a catalog table's segment granularity to be hourly, may assign the segment granularity property of the table to be eitherPT1H, orHOUR. These are the same formats accepted at query time.This PR has: