From ccdbc746c1ff2690f94f0b6f15ed1a7b041a1a19 Mon Sep 17 00:00:00 2001 From: zachjsh Date: Mon, 10 Jun 2024 16:29:40 -0400 Subject: [PATCH 1/4] 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/4] * 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/4] * 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 e8d791cd87b9daf41be3a082cda0a11d17e2247a Mon Sep 17 00:00:00 2001 From: zachjsh Date: Tue, 12 Nov 2024 23:05:10 -0500 Subject: [PATCH 4/4] Add 'ingest/notices/time' metric to statsd emitter This metric gives the milliseconds taken to process a notice by the supervisor. --- .../src/main/resources/defaultMetricDimensions.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json b/extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json index f39e9493dbf6..91a74707e4f0 100644 --- a/extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json +++ b/extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json @@ -65,6 +65,8 @@ "ingest/kinesis/avgLag/time" : { "dimensions" : ["dataSource", "stream"], "type" : "gauge" }, "ingest/kinesis/partitionLag/time" : { "dimensions" : ["dataSource", "stream", "partition"], "type" : "gauge" }, + "ingest/notices/time" : { "dimensions" : ["dataSource", "noticeType"], "type" : "timer" }, + "task/success/count" : { "dimensions" : ["dataSource"], "type" : "count" }, "task/failed/count" : { "dimensions" : ["dataSource"], "type" : "count" }, "task/running/count" : { "dimensions" : ["dataSource"], "type" : "gauge" },