From ccdbc746c1ff2690f94f0b6f15ed1a7b041a1a19 Mon Sep 17 00:00:00 2001 From: zachjsh Date: Mon, 10 Jun 2024 16:29:40 -0400 Subject: [PATCH 1/6] SQL syntax error should target USER persona --- .../druid/sql/calcite/planner/DruidPlanner.java | 8 +------- .../druid/sql/calcite/planner/QueryHandler.java | 11 +---------- .../druid/sql/calcite/BaseCalciteQueryTest.java | 6 +----- 3 files changed, 3 insertions(+), 22 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java index 4b697a0d5dfa..cf1d22eb39b4 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java @@ -380,13 +380,7 @@ public static DruidException translateException(Exception e) } } - return DruidException.forPersona(DruidException.Persona.DEVELOPER) - .ofCategory(DruidException.Category.UNCATEGORIZED) - .build( - inner, - "Unable to parse the SQL, unrecognized error from calcite: [%s]", - inner.getMessage() - ); + return InvalidSqlInput.exception(inner.getMessage()); } catch (RelOptPlanner.CannotPlanException inner) { return DruidException.forPersona(DruidException.Persona.USER) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java index 9f15d3822866..2d292621b57d 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java @@ -684,16 +684,7 @@ private DruidException buildSQLPlanningError(RelOptPlanner.CannotPlanException e .ofCategory(DruidException.Category.UNSUPPORTED) .build(exception, "Unhandled Query Planning Failure, see broker logs for details"); } else { - // Planning errors are more like hints: it isn't guaranteed that the planning error is actually what went wrong. - // For this reason, we consider these as targetting a more expert persona, i.e. the admin instead of the actual - // user. - throw DruidException.forPersona(DruidException.Persona.ADMIN) - .ofCategory(DruidException.Category.INVALID_INPUT) - .build( - exception, - "Query could not be planned. A possible reason is [%s]", - errorMessage - ); + throw InvalidSqlInput.exception("Query could not be planned. A possible reason is [%s]", errorMessage); } } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java index dfee7d0e3a22..9c34c89bd8fd 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java @@ -664,11 +664,7 @@ public void assertQueryIsUnplannable(final PlannerConfig plannerConfig, final St private DruidExceptionMatcher buildUnplannableExceptionMatcher() { - if (testBuilder().isDecoupledMode()) { - return new DruidExceptionMatcher(Persona.USER, Category.INVALID_INPUT, "invalidInput"); - } else { - return new DruidExceptionMatcher(Persona.ADMIN, Category.INVALID_INPUT, "general"); - } + return new DruidExceptionMatcher(Persona.USER, Category.INVALID_INPUT, "invalidInput"); } /** From abe109262edf128e67f6373b96dc62ede370e3ea Mon Sep 17 00:00:00 2001 From: zachjsh Date: Tue, 11 Jun 2024 14:34:12 -0400 Subject: [PATCH 2/6] * revert change to queryHandler and related tests, based on review comments --- .../druid/sql/calcite/planner/QueryHandler.java | 11 ++++++++++- .../druid/sql/calcite/BaseCalciteQueryTest.java | 6 +++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java index 2d292621b57d..9f15d3822866 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java @@ -684,7 +684,16 @@ private DruidException buildSQLPlanningError(RelOptPlanner.CannotPlanException e .ofCategory(DruidException.Category.UNSUPPORTED) .build(exception, "Unhandled Query Planning Failure, see broker logs for details"); } else { - throw InvalidSqlInput.exception("Query could not be planned. A possible reason is [%s]", errorMessage); + // Planning errors are more like hints: it isn't guaranteed that the planning error is actually what went wrong. + // For this reason, we consider these as targetting a more expert persona, i.e. the admin instead of the actual + // user. + throw DruidException.forPersona(DruidException.Persona.ADMIN) + .ofCategory(DruidException.Category.INVALID_INPUT) + .build( + exception, + "Query could not be planned. A possible reason is [%s]", + errorMessage + ); } } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java index 9c34c89bd8fd..dfee7d0e3a22 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java @@ -664,7 +664,11 @@ public void assertQueryIsUnplannable(final PlannerConfig plannerConfig, final St private DruidExceptionMatcher buildUnplannableExceptionMatcher() { - return new DruidExceptionMatcher(Persona.USER, Category.INVALID_INPUT, "invalidInput"); + if (testBuilder().isDecoupledMode()) { + return new DruidExceptionMatcher(Persona.USER, Category.INVALID_INPUT, "invalidInput"); + } else { + return new DruidExceptionMatcher(Persona.ADMIN, Category.INVALID_INPUT, "general"); + } } /** From 5d730475dca1c90cc2fd4abc15d890a1d812acb1 Mon Sep 17 00:00:00 2001 From: zachjsh Date: Tue, 11 Jun 2024 15:58:16 -0400 Subject: [PATCH 3/6] * add test --- .../druid/sql/calcite/CalciteInsertDmlTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java index bb9c03aa3c88..b40a4c87c3ab 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java @@ -1626,6 +1626,18 @@ public void testInsertWithInvalidSelectStatement() .verify(); } + @Test + public void testInsertWithLongIdentifer() + { + // This test fails because an identifer is specified of length 200, which exceeds the length limit of 128 + // characters. + String longIdentifer = new String(new char[200]).replace('\0', 'a'); + testIngestionQuery() + .sql(StringUtils.format("INSERT INTO t SELECT %s FROM foo PARTITIONED BY ALL", longIdentifer)) // count is a keyword + .expectValidationError(invalidSqlContains(StringUtils.format("Length of identifier '%s' must be less than or equal to 128 characters", longIdentifer))) + .verify(); + } + @Test public void testInsertWithUnnamedColumnInSelectStatement() { From f788157ffbc6972373f3d090b338c3f747ec29fb Mon Sep 17 00:00:00 2001 From: zachjsh Date: Mon, 1 Jul 2024 19:40:29 -0400 Subject: [PATCH 4/6] Allow segment granularity defined in catalog to be in same format as query --- .../druid/catalog/model/CatalogUtils.java | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) 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 841d592062bf..2d295372d662 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 @@ -68,12 +68,20 @@ public static Granularity asDruidGranularity(String value) if (Strings.isNullOrEmpty(value) || 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(StringUtils.format("[%s] is an invalid granularity string", value)); + } } + + return granularity; } /** @@ -275,18 +283,12 @@ public static Map mergeProperties( 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. " From d703439eb27e91bdfc346e836d454a86bade10f0 Mon Sep 17 00:00:00 2001 From: zachjsh Date: Mon, 1 Jul 2024 21:25:16 -0400 Subject: [PATCH 5/6] * address review comments --- .../testsEx/catalog/ITCatalogIngestAndQueryTest.java | 4 ++-- .../java/org/apache/druid/catalog/model/CatalogUtils.java | 8 +++++--- .../druid/catalog/model/facade/DatasourceFacade.java | 3 +++ 3 files changed, 10 insertions(+), 5 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 3dc3c4f5d90f..29dee7329cb0 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 void testInsertImplicitCast() throws Exception { 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 void testInsertWithClusteringFromCatalog() throws Exception { 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 2d295372d662..c0e0a320a8e6 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 @@ -23,6 +23,7 @@ import com.google.common.base.Strings; import org.apache.druid.catalog.model.ModelProperties.PropertyDefn; import org.apache.druid.catalog.model.table.DatasourceDefn; +import org.apache.druid.error.InvalidInput; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.ISE; @@ -33,6 +34,7 @@ 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,9 +65,9 @@ public static List columnNames(List columns) * 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; @@ -77,7 +79,7 @@ public static Granularity asDruidGranularity(String value) granularity = new PeriodGranularity(new Period(value), null, null); } catch (IllegalArgumentException e2) { - throw new IAE(StringUtils.format("[%s] is an invalid granularity string", value)); + throw InvalidInput.exception("[%s] is an invalid granularity string.", value); } } 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 7ac00d9b608a..9e4c2d9df6ad 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.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 String segmentGranularityString() return stringProperty(DatasourceDefn.SEGMENT_GRANULARITY_PROPERTY); } + @Nullable public Granularity segmentGranularity() { String definedGranularity = segmentGranularityString(); From ba83f85cec8805053ea2ea9ea5187cb65bb0fa14 Mon Sep 17 00:00:00 2001 From: zachjsh Date: Tue, 2 Jul 2024 00:59:30 -0400 Subject: [PATCH 6/6] * fix failing test. --- .../main/java/org/apache/druid/catalog/model/CatalogUtils.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 c0e0a320a8e6..d0ac6c31e76b 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 @@ -23,7 +23,6 @@ import com.google.common.base.Strings; import org.apache.druid.catalog.model.ModelProperties.PropertyDefn; import org.apache.druid.catalog.model.table.DatasourceDefn; -import org.apache.druid.error.InvalidInput; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.ISE; @@ -79,7 +78,7 @@ public static Granularity asDruidGranularity(@Nonnull String value) granularity = new PeriodGranularity(new Period(value), null, null); } catch (IllegalArgumentException e2) { - throw InvalidInput.exception("[%s] is an invalid granularity string.", value); + throw new IAE("[%s] is an invalid granularity string.", value); } }