You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/11/19 21:52:21 UTC
[GitHub] [druid] gianm opened a new pull request #11959: SQL INSERT planner support.
gianm opened a new pull request #11959:
URL: https://github.com/apache/druid/pull/11959
This patch is doing parts 1–3 and 5 from the "Proposed changes" in #11929. Nothing is exposed to end users as a result of this patch. It's just changes to the planner that enable understanding and authorizing INSERTs, and writing some initial tests. The main logic is in **DruidPlanner** and the main tests are in **CalciteInsertDmlTest**. Major things you _won't_ find are the ability to actually execute INSERT queries (that needs a new QueryMaker) and the ability to handle PARTITION BY and CLUSTER BY (that will require parser changes). Those would be done in follow-on patches.
The main changes are:
1) DruidPlanner is able to validate and authorize INSERT queries. They require WRITE permission on the target datasource.
2) QueryMaker is now an interface, and there is a QueryMakerFactory that creates instances of it. There is only one production implementation of each (NativeQueryMaker and NativeQueryMakerFactory), which together behave the same way as the former QueryMaker class. But this opens the door to executing queries in ways other than the Druid query stack, and is used by unit tests (CalciteInsertDmlTest) to test the INSERT planning functionality.
3) Adds an EXTERN table macro that allows references external data using InputSource and InputFormat from Druid's batch ingestion API. This is not exposed in production yet, but is used by unit tests.
4) Adds a QueryFeature concept that enables the planner to change its behavior slightly depending on the capabilities of the execution system.
5) Adds an "AuthorizableOperator" concept that enables SqlOperators to require additional permissions. This is used by the EXTERN table macro.
Related odds and ends:
- Add equals, hashCode, toString methods to InlineInputSource. Aids in the "from external" tests in CalciteInsertDmlTest.
- Add JSON-serializability to RowSignature.
- Move the SQL string inside PlannerContext so it is "baked into" the planner when the planner is created. Cleans up the code a bit, since in practice, the same query is passed in every time to the same planner anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11959: SQL INSERT planner support.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11959:
URL: https://github.com/apache/druid/pull/11959#discussion_r755894673
##########
File path: sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java
##########
@@ -1097,9 +1101,24 @@ public void testParameterBinding() throws Exception
}
@Test
- public void testSysTableParameterBinding() throws Exception
+ public void testSysTableParameterBindingRegularUser() throws Exception
{
- PreparedStatement statement = client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");
+ PreparedStatement statement =
+ client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");
Review comment:
ah nevermind, found the change in `CalciteTests` that limits to datasource and view permissions, leaving here in case anyone else wonders the same thing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #11959: SQL INSERT planner support.
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #11959:
URL: https://github.com/apache/druid/pull/11959
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #11959: SQL INSERT planner support.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #11959:
URL: https://github.com/apache/druid/pull/11959#discussion_r756220470
##########
File path: sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java
##########
@@ -1097,9 +1101,24 @@ public void testParameterBinding() throws Exception
}
@Test
- public void testSysTableParameterBinding() throws Exception
+ public void testSysTableParameterBindingRegularUser() throws Exception
{
- PreparedStatement statement = client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");
+ PreparedStatement statement =
+ client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");
Review comment:
Ah, yeah, this is happening because I made the test authorizer more strict for the "regular user". The regular user now only has permissions to datasources and views, not state or external data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11959: SQL INSERT planner support.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11959:
URL: https://github.com/apache/druid/pull/11959#discussion_r755849785
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -83,115 +95,120 @@
private final FrameworkConfig frameworkConfig;
private final Planner planner;
private final PlannerContext plannerContext;
- private final ObjectMapper jsonMapper;
+ private final QueryMakerFactory queryMakerFactory;
+
private RexBuilder rexBuilder;
- public DruidPlanner(
+ DruidPlanner(
final FrameworkConfig frameworkConfig,
final PlannerContext plannerContext,
- final ObjectMapper jsonMapper
+ final QueryMakerFactory queryMakerFactory
)
{
this.frameworkConfig = frameworkConfig;
this.planner = Frameworks.getPlanner(frameworkConfig);
this.plannerContext = plannerContext;
- this.jsonMapper = jsonMapper;
+ this.queryMakerFactory = queryMakerFactory;
}
/**
- * Validates an SQL query and collects a {@link ValidationResult} which contains a set of
- * {@link org.apache.druid.server.security.Resource} corresponding to any Druid datasources or views which are taking
- * part in the query
+ * Validates a SQL query and populates {@link PlannerContext#getResourceActions()}.
Review comment:
As an “es-cue-el” person, this will never read right to me :trollface:
On a serious note, the refactors in `DruidPlanner` are nice :+1:
##########
File path: sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java
##########
@@ -1097,9 +1101,24 @@ public void testParameterBinding() throws Exception
}
@Test
- public void testSysTableParameterBinding() throws Exception
+ public void testSysTableParameterBindingRegularUser() throws Exception
{
- PreparedStatement statement = client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");
+ PreparedStatement statement =
+ client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");
Review comment:
is it a bug that this didn't explode before or is it an artifact of changes elsewhere or just a flaw in the original test?
##########
File path: sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java
##########
@@ -1097,9 +1101,24 @@ public void testParameterBinding() throws Exception
}
@Test
- public void testSysTableParameterBinding() throws Exception
+ public void testSysTableParameterBindingRegularUser() throws Exception
{
- PreparedStatement statement = client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");
+ PreparedStatement statement =
+ client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");
Review comment:
ah nevermind, found the change in `CalciteTests` that limits to datasource and view permissions
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidOuterQueryRel.java
##########
@@ -85,21 +84,6 @@ public PartialDruidQuery getPartialDruidQuery()
return partialQuery;
}
- @Override
- public Sequence<Object[]> runQuery()
- {
- // runQuery doesn't need to finalize aggregations, because the fact that runQuery is happening suggests this
- // is the outermost query and it will actually get run as a native query. Druid's native query layer will
- // finalize aggregations for the outermost query even if we don't explicitly ask it to.
-
- final DruidQuery query = toDruidQuery(false);
- if (query != null) {
Review comment:
should this null check be moved into `NativeQueryMaker` or the `DruidRel` method or can it not be null here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #11959: SQL INSERT planner support.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #11959:
URL: https://github.com/apache/druid/pull/11959#discussion_r756218455
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidOuterQueryRel.java
##########
@@ -85,21 +84,6 @@ public PartialDruidQuery getPartialDruidQuery()
return partialQuery;
}
- @Override
- public Sequence<Object[]> runQuery()
- {
- // runQuery doesn't need to finalize aggregations, because the fact that runQuery is happening suggests this
- // is the outermost query and it will actually get run as a native query. Druid's native query layer will
- // finalize aggregations for the outermost query even if we don't explicitly ask it to.
-
- final DruidQuery query = toDruidQuery(false);
- if (query != null) {
Review comment:
It can't be null; that was a hold-over from an earlier time when `null` was used by the old semi-join rel to mean "known to be empty". The new join rel doesn't work that way, since it pushes computation down into the native layer. These days, `DruidRel.toDruidQuery` is not `@Nullable` anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #11959: SQL INSERT planner support.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #11959:
URL: https://github.com/apache/druid/pull/11959#discussion_r756218455
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidOuterQueryRel.java
##########
@@ -85,21 +84,6 @@ public PartialDruidQuery getPartialDruidQuery()
return partialQuery;
}
- @Override
- public Sequence<Object[]> runQuery()
- {
- // runQuery doesn't need to finalize aggregations, because the fact that runQuery is happening suggests this
- // is the outermost query and it will actually get run as a native query. Druid's native query layer will
- // finalize aggregations for the outermost query even if we don't explicitly ask it to.
-
- final DruidQuery query = toDruidQuery(false);
- if (query != null) {
Review comment:
It can't be null; that was a hold-over from an earlier time when `null` was used by the old semi-join rel to mean "known to be empty". The new join rel doesn't work that way, since it pushes computation down into the native layer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org