From 2734fa5fd41448925b3a084b663f2ba5ddcd21d8 Mon Sep 17 00:00:00 2001 From: Soumyava Das Date: Wed, 5 Jul 2023 16:10:20 -0700 Subject: [PATCH 1/2] Handling a case where a ValueInstantiationException was converted to a RuntimeException. Now the RuntimeException is being replaced by an user facing DruidException of Invalid category which would allow calcite not to throw an uncategorized exception --- .../external/ExternalOperatorConversion.java | 3 ++- .../sql/calcite/CalciteInsertDmlTest.java | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java index f2f667af4e31..b9c0e4f55d22 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java @@ -29,6 +29,7 @@ import org.apache.druid.catalog.model.table.ExternalTableSpec; import org.apache.druid.data.input.InputFormat; import org.apache.druid.data.input.InputSource; +import org.apache.druid.error.DruidException; import org.apache.druid.guice.annotations.Json; import org.apache.druid.java.util.common.IAE; import org.apache.druid.segment.column.RowSignature; @@ -120,7 +121,7 @@ public ExternalTableSpec apply( ); } catch (JsonProcessingException e) { - throw new RuntimeException(e); + throw DruidException.forPersona(DruidException.Persona.USER).ofCategory(DruidException.Category.INVALID_INPUT).build(e, e.getMessage()); } } } 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 1679b86fb069..d180e11e0012 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 @@ -1410,4 +1410,26 @@ public void testInsertWithSqlOuterLimit() ) .verify(); } + + @Test + public void testSyntaxError() + { + final String sqlString = "insert into test_talaria_test_extern_extern_cols \n" + + "select time_parse(\"time\") as __time, * \n" + + "from table( \n" + + "extern(\n" + + "'{\"type\": \"s3\", \"uris\": [\\\"s3://imply-eng-datasets/qa/IngestionTest/wikipedia/files/wikiticker-2015-09-12-sampled.mini.json.gz\\\"]}',\n" + + "'{\"type\\\": \"json\"}',\n" + + "'[{\"name\": \"time\", \"type\": \"string\"}, {\"name\": \"channel\", \"type\": \"string\"}, {\"countryName\": \"string\"}]'\n" + + ")\n" + + ")\n" + + "partitioned by DAY\n" + + "clustered by channel"; + HashMap context = new HashMap<>(DEFAULT_CONTEXT); + context.put(PlannerContext.CTX_SQL_OUTER_LIMIT, 100); + testIngestionQuery().context(context).sql(sqlString).expectValidationError( + invalidSqlIs("Context parameter [sqlOuterLimit] cannot be provided on operator [INSERT]") + ) + .verify(); + } } From df3bc2a9c24b403a1f1fbf24c284e89ef5cb3728 Mon Sep 17 00:00:00 2001 From: Soumyava Das Date: Wed, 5 Jul 2023 16:16:35 -0700 Subject: [PATCH 2/2] Better test case and better formatting --- .../external/ExternalOperatorConversion.java | 4 +++- .../sql/calcite/CalciteInsertDmlTest.java | 18 +++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java index b9c0e4f55d22..26fdd514e8f6 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java @@ -121,7 +121,9 @@ public ExternalTableSpec apply( ); } catch (JsonProcessingException e) { - throw DruidException.forPersona(DruidException.Persona.USER).ofCategory(DruidException.Category.INVALID_INPUT).build(e, e.getMessage()); + throw DruidException.forPersona(DruidException.Persona.USER) + .ofCategory(DruidException.Category.INVALID_INPUT) + .build(e, e.getMessage()); } } } 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 d180e11e0012..71672ae8d510 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 @@ -1412,14 +1412,14 @@ public void testInsertWithSqlOuterLimit() } @Test - public void testSyntaxError() + public void testErrorWithUnableToConstructColumnSignatureWithExtern() { - final String sqlString = "insert into test_talaria_test_extern_extern_cols \n" + final String sqlString = "insert into dst \n" + "select time_parse(\"time\") as __time, * \n" + "from table( \n" + "extern(\n" + "'{\"type\": \"s3\", \"uris\": [\\\"s3://imply-eng-datasets/qa/IngestionTest/wikipedia/files/wikiticker-2015-09-12-sampled.mini.json.gz\\\"]}',\n" - + "'{\"type\\\": \"json\"}',\n" + + "'{\"type\": \"json\"}',\n" + "'[{\"name\": \"time\", \"type\": \"string\"}, {\"name\": \"channel\", \"type\": \"string\"}, {\"countryName\": \"string\"}]'\n" + ")\n" + ")\n" @@ -1427,8 +1427,16 @@ public void testSyntaxError() + "clustered by channel"; HashMap context = new HashMap<>(DEFAULT_CONTEXT); context.put(PlannerContext.CTX_SQL_OUTER_LIMIT, 100); - testIngestionQuery().context(context).sql(sqlString).expectValidationError( - invalidSqlIs("Context parameter [sqlOuterLimit] cannot be provided on operator [INSERT]") + testIngestionQuery().context(context).sql(sqlString) + .expectValidationError( + new DruidExceptionMatcher( + DruidException.Persona.USER, + DruidException.Category.INVALID_INPUT, + "general" + ) + .expectMessageContains( + "Cannot construct instance of `org.apache.druid.segment.column.ColumnSignature`, problem: `java.lang.NullPointerException`\n" + ) ) .verify(); }