From 5e05858ff7d3a79addd6710030b6289cb3dc1cc4 Mon Sep 17 00:00:00 2001
From: zachjsh <zachjsh@gmail.com>
Date: Tue, 2 Jul 2024 12:14:28 -0400
Subject: [PATCH] Catalog granularity accepts query format (#16680)

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 the `Granularity.fromString(str)` method, or `new PeriodGranularity(new Period(value), null, null)`, and that granularity maps to a standard supported granularity, where `GranularityType.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 either `PT1H`, or `HOUR`. These are the same formats accepted at query time.
---
 .../catalog/ITCatalogIngestAndQueryTest.java  |  4 +--
 .../druid/catalog/model/CatalogUtils.java     | 27 ++++++++++---------
 .../model/facade/DatasourceFacade.java        |  3 +++
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/catalog/ITCatalogIngestAndQueryTest.java b/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/catalog/ITCatalogIngestAndQueryTest.java
index 3dc3c4f5d90..29dee7329cb 100644
--- a/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/catalog/ITCatalogIngestAndQueryTest.java
+++ b/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/catalog/ITCatalogIngestAndQueryTest.java
@@ -109,7 +109,7 @@ public abstract class ITCatalogIngestAndQueryTest
   {
     String queryFile = "/catalog/implicitCast_select.sql";
     String tableName = "testImplicitCast" + operationName;
-    TableMetadata table = TableBuilder.datasource(tableName, "P1D")
+    TableMetadata table = TableBuilder.datasource(tableName, "DAY")
         .column(Columns.TIME_COLUMN, Columns.LONG)
         .column("double_col1", "DOUBLE")
         .build();
@@ -179,7 +179,7 @@ public abstract class ITCatalogIngestAndQueryTest
   {
     String queryFile = "/catalog/clustering_select.sql";
     String tableName = "testWithClusteringFromCatalog" + operationName;
-    TableMetadata table = TableBuilder.datasource(tableName, "P1D")
+    TableMetadata table = TableBuilder.datasource(tableName, "ALL")
         .column(Columns.TIME_COLUMN, Columns.LONG)
         .column("bigint_col1", "BIGINT")
         .property(
diff --git a/server/src/main/java/org/apache/druid/catalog/model/CatalogUtils.java b/server/src/main/java/org/apache/druid/catalog/model/CatalogUtils.java
index 841d592062b..d0ac6c31e76 100644
--- a/server/src/main/java/org/apache/druid/catalog/model/CatalogUtils.java
+++ b/server/src/main/java/org/apache/druid/catalog/model/CatalogUtils.java
@@ -33,6 +33,7 @@ import org.apache.druid.java.util.common.granularity.GranularityType;
 import org.apache.druid.java.util.common.granularity.PeriodGranularity;
 import org.joda.time.Period;
 
+import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
 import java.net.URI;
@@ -63,17 +64,25 @@ public class CatalogUtils
    * For the odd interval, the interval name is also accepted (for the other
    * intervals, the interval name is the descriptive string).
    */
-  public static Granularity asDruidGranularity(String value)
+  public static Granularity asDruidGranularity(@Nonnull String value)
   {
-    if (Strings.isNullOrEmpty(value) || value.equalsIgnoreCase(DatasourceDefn.ALL_GRANULARITY)) {
+    if (value.equalsIgnoreCase(DatasourceDefn.ALL_GRANULARITY)) {
       return Granularities.ALL;
     }
+    Granularity granularity;
     try {
-      return new PeriodGranularity(new Period(value), null, null);
+      granularity = Granularity.fromString(value);
     }
     catch (IllegalArgumentException e) {
-      throw new IAE(StringUtils.format("'%s' is an invalid period string", value));
+      try {
+        granularity = new PeriodGranularity(new Period(value), null, null);
+      }
+      catch (IllegalArgumentException e2) {
+        throw new IAE("[%s] is an invalid granularity string.", value);
+      }
     }
+
+    return granularity;
   }
 
   /**
@@ -275,18 +284,12 @@ public class CatalogUtils
     return merged;
   }
 
-  public static void validateGranularity(String value)
+  public static void validateGranularity(final String value)
   {
     if (value == null) {
       return;
     }
-    Granularity granularity;
-    try {
-      granularity = new PeriodGranularity(new Period(value), null, null);
-    }
-    catch (IllegalArgumentException e) {
-      throw new IAE(StringUtils.format("[%s] is an invalid granularity string", value));
-    }
+    final Granularity granularity = asDruidGranularity(value);
     if (!GranularityType.isStandard(granularity)) {
       throw new IAE(
           "Unsupported segment graularity. "
diff --git a/server/src/main/java/org/apache/druid/catalog/model/facade/DatasourceFacade.java b/server/src/main/java/org/apache/druid/catalog/model/facade/DatasourceFacade.java
index 7ac00d9b608..9e4c2d9df6a 100644
--- a/server/src/main/java/org/apache/druid/catalog/model/facade/DatasourceFacade.java
+++ b/server/src/main/java/org/apache/druid/catalog/model/facade/DatasourceFacade.java
@@ -30,6 +30,8 @@ import org.apache.druid.java.util.common.granularity.Granularity;
 import org.apache.druid.java.util.common.logger.Logger;
 import org.apache.druid.segment.column.ColumnType;
 
+import javax.annotation.Nullable;
+
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -122,6 +124,7 @@ public class DatasourceFacade extends TableFacade
     return stringProperty(DatasourceDefn.SEGMENT_GRANULARITY_PROPERTY);
   }
 
+  @Nullable
   public Granularity segmentGranularity()
   {
     String definedGranularity = segmentGranularityString();