You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "adarshsanjeev (via GitHub)" <gi...@apache.org> on 2023/04/21 04:27:37 UTC
[GitHub] [druid] adarshsanjeev opened a new pull request, #13674: Add delete syntax
adarshsanjeev opened a new pull request, #13674:
URL: https://github.com/apache/druid/pull/13674
This is currently a draft PR.
This PR aims to improve the syntax for commonly used INSERT and REPLACE queries by adding new syntax that is more in line with the intention of the user.
Currently, there is a need to delete all rows which match a certain condition, the user needs to run
> REPLACE INTO datasource
> SELECT * FROM datasource
> WHERE ! \<condition\>
> PARTITIONED BY <new partitioning>
> [CLUSTERED BY <new clustering>]
A much more intuitive format would be
> DELETE FROM datasource
> WHERE \<condition\>
> PARTITIONED BY <new partitioning>
> [CLUSTERED BY <new clustering>]
There is no new capabilities being added here, this is simply a better syntax for existing queries.
Therefore, it is important to note that this is not a 'true' DELETE like in SQL. DELETE would still trigger a reingest of the datasource filtering out any rows that match the condition. There is some scope for improvement in the future depending on the condition here (such as directly dropping a segment if the condition aligns with the partition boundaries).
The partitioning and clustering is still required as delete still performs a reingest of the table behind the scenes, but this might go away once the catalog PR is merged.
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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
Re: [PR] Add delete syntax (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #13674: Add delete syntax
URL: https://github.com/apache/druid/pull/13674
--
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] adarshsanjeev closed pull request #13674: Add delete syntax
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev closed pull request #13674: Add delete syntax
URL: https://github.com/apache/druid/pull/13674
--
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] paul-rogers commented on a diff in pull request #13674: Add delete syntax
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on code in PR #13674:
URL: https://github.com/apache/druid/pull/13674#discussion_r1088448822
##########
sql/src/main/codegen/includes/delete.ftl:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// Taken from syntax of SqlInsert statement from calcite parser, edited for replace syntax
+SqlNode DruidSqlDeleteEof() :
+{
+ SqlNode table;
+ SqlNode source = null;
+ final Span s;
+ // Using fully qualified name for Pair class, since Calcite also has a same class name being used in the Parser.jj
+ org.apache.druid.java.util.common.Pair<Granularity, String> partitionedBy = new org.apache.druid.java.util.common.Pair(null, null);
+ SqlNodeList clusteredBy = null;
+}
+{
+ <DELETE> { s = span(); }
+ <FROM>
+ table = CompoundIdentifier()
+ [
+ <WHERE> source = Expression(ExprContext.ACCEPT_SUB_QUERY)
+ ]
+ [
+ <PARTITIONED> <BY>
+ partitionedBy = PartitionGranularity()
+ ]
+ [
+ <CLUSTERED> <BY>
+ clusteredBy = ClusterItems()
Review Comment:
If our code that does delete needs partitioning, then that code is, I would assert, broken and needs to be fixed. Let's not add SQL syntax exposes partitioning that isn't actually needed.
Let's imagine some cases. I create segments with 1-hour partitioning. Then, I create more segments, in the same time period, with 1 day partitioning. Now, I realize I messed up and want to delete this entire group of segments. Quick: which partitioning do I use? Hour? Or Day? Or, do I need different delete tasks, one for the hour segments, one for Day? Is this the user experience we want?
Often, just writing down the proposed syntax, and thinking through the user experience, can save us lots of back-and-forth at the level of individual lines of code.
So, I propose we either fix the need for partitioning in the reingest, or we give it a fake partitioning. Remember: there is no new data to partition.
--
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] paul-rogers commented on pull request #13674: Add delete syntax
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on PR #13674:
URL: https://github.com/apache/druid/pull/13674#issuecomment-1411068215
Yet another suggestion. We have the `WHERE` clause: `WHERE __time >= ? AND __time < ?`, for two parameters we'll call `start` and `end`. This actually can help us infer the time partitioning. The time interval has to be a multiple of one of our supported time partitions. Perhaps we can do a bit of math to work out the partitioning if we need it. Or, at least, _a_ partitioning. For example, if `end` - `start` is 3 hours, infer partitioning is 1 hour. I suspect that, even if partitioning is DAY, and delete says it is HOUR, it will still work. The result is that the user doesn't have to tell Druid what time partitioning Druid has used to store Druid's segments.
For clustering, it is probably OK to omit clustering and just "cluster" all deleted segments into one big (empty) cluster.
--
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
Re: [PR] Add delete syntax (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13674:
URL: https://github.com/apache/druid/pull/13674#issuecomment-1920235259
This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.
--
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
Re: [PR] Add delete syntax (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13674:
URL: https://github.com/apache/druid/pull/13674#issuecomment-1972195652
This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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] paul-rogers commented on pull request #13674: Add delete syntax
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #13674:
URL: https://github.com/apache/druid/pull/13674#issuecomment-1387496613
This is a change to SQL syntax. SQL syntax changes typically involve much discussion as we want to make the syntax a SQL-compatible as possible, but we usually need some Druid-specific bits. While this is a draft, please add to the description the specific syntax being proposed.
--
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] paul-rogers commented on a diff in pull request #13674: Add delete syntax
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on code in PR #13674:
URL: https://github.com/apache/druid/pull/13674#discussion_r1073897566
##########
sql/src/main/codegen/includes/delete.ftl:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// Taken from syntax of SqlInsert statement from calcite parser, edited for replace syntax
+SqlNode DruidSqlDeleteEof() :
+{
+ SqlNode table;
+ SqlNode source = null;
+ final Span s;
+ // Using fully qualified name for Pair class, since Calcite also has a same class name being used in the Parser.jj
+ org.apache.druid.java.util.common.Pair<Granularity, String> partitionedBy = new org.apache.druid.java.util.common.Pair(null, null);
+ SqlNodeList clusteredBy = null;
+}
+{
+ <DELETE> { s = span(); }
+ <FROM>
+ table = CompoundIdentifier()
+ [
+ <WHERE> source = Expression(ExprContext.ACCEPT_SUB_QUERY)
+ ]
+ [
+ <PARTITIONED> <BY>
+ partitionedBy = PartitionGranularity()
Review Comment:
Note that this syntax is changing a bit in this PR: https://github.com/apache/druid/pull/13686
##########
sql/src/main/codegen/includes/delete.ftl:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// Taken from syntax of SqlInsert statement from calcite parser, edited for replace syntax
+SqlNode DruidSqlDeleteEof() :
+{
+ SqlNode table;
+ SqlNode source = null;
+ final Span s;
+ // Using fully qualified name for Pair class, since Calcite also has a same class name being used in the Parser.jj
+ org.apache.druid.java.util.common.Pair<Granularity, String> partitionedBy = new org.apache.druid.java.util.common.Pair(null, null);
+ SqlNodeList clusteredBy = null;
+}
+{
+ <DELETE> { s = span(); }
+ <FROM>
+ table = CompoundIdentifier()
+ [
+ <WHERE> source = Expression(ExprContext.ACCEPT_SUB_QUERY)
+ ]
+ [
+ <PARTITIONED> <BY>
+ partitionedBy = PartitionGranularity()
+ ]
+ [
+ <CLUSTERED> <BY>
+ clusteredBy = ClusterItems()
Review Comment:
Do we need clustering for delete? What does it mean to cluster deletions?
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/IngestHandler.java:
##########
@@ -344,4 +345,55 @@ public void validate() throws ValidationException
}
}
}
+
+ /**
+ * Handler for the DELETE statement.
+ */
+ protected static class DeleteHandler extends IngestHandler
+ {
+ private final DruidSqlDelete sqlNode;
+
+ public DeleteHandler(
+ SqlStatementHandler.HandlerContext handlerContext,
+ DruidSqlDelete sqlNode,
+ SqlExplain explain
+ ) throws ValidationException
+ {
+ super(
+ handlerContext,
+ sqlNode,
+ convertQuery(sqlNode),
+ explain
+ );
+ this.sqlNode = sqlNode;
+ }
+
+ @Override
+ public SqlNode sqlNode()
+ {
+ return sqlNode;
+ }
+
+ @Override
+ protected DruidSqlIngest ingestNode()
+ {
+ return sqlNode;
+ }
+
+ @Override
+ public void validate() throws ValidationException
+ {
+ if (!handlerContext.plannerContext().engineHasFeature(EngineFeature.CAN_REPLACE)) {
+ throw new ValidationException(StringUtils.format(
+ "Cannot execute REPLACE with SQL engine '%s'.",
Review Comment:
Cannot execute `DELETE` ...
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -158,6 +159,8 @@ private SqlStatementHandler createHandler(final SqlNode node) throws ValidationE
return new IngestHandler.InsertHandler(handlerContext, (DruidSqlInsert) query, explain);
} else if (query instanceof DruidSqlReplace) {
return new IngestHandler.ReplaceHandler(handlerContext, (DruidSqlReplace) query, explain);
+ } else if (query instanceof DruidSqlDelete) {
+ return new IngestHandler.DeleteHandler(handlerContext, (DruidSqlDelete) query, explain);
Review Comment:
Thanks for creating a new handler: that is definitely the way to do this task.
--
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] adarshsanjeev commented on a diff in pull request #13674: Add delete syntax
Posted by GitBox <gi...@apache.org>.
adarshsanjeev commented on code in PR #13674:
URL: https://github.com/apache/druid/pull/13674#discussion_r1080913156
##########
sql/src/main/codegen/includes/delete.ftl:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// Taken from syntax of SqlInsert statement from calcite parser, edited for replace syntax
+SqlNode DruidSqlDeleteEof() :
+{
+ SqlNode table;
+ SqlNode source = null;
+ final Span s;
+ // Using fully qualified name for Pair class, since Calcite also has a same class name being used in the Parser.jj
+ org.apache.druid.java.util.common.Pair<Granularity, String> partitionedBy = new org.apache.druid.java.util.common.Pair(null, null);
+ SqlNodeList clusteredBy = null;
+}
+{
+ <DELETE> { s = span(); }
+ <FROM>
+ table = CompoundIdentifier()
+ [
+ <WHERE> source = Expression(ExprContext.ACCEPT_SUB_QUERY)
+ ]
+ [
+ <PARTITIONED> <BY>
+ partitionedBy = PartitionGranularity()
Review Comment:
Thanks for the heads up. I'm looking forward to the catalog changes, I think this would make the delete syntax (and all the others actually) a lot more "SQL-like" since we can drop the partitioned by as a necessary part of the query.
##########
sql/src/main/codegen/includes/delete.ftl:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// Taken from syntax of SqlInsert statement from calcite parser, edited for replace syntax
+SqlNode DruidSqlDeleteEof() :
+{
+ SqlNode table;
+ SqlNode source = null;
+ final Span s;
+ // Using fully qualified name for Pair class, since Calcite also has a same class name being used in the Parser.jj
+ org.apache.druid.java.util.common.Pair<Granularity, String> partitionedBy = new org.apache.druid.java.util.common.Pair(null, null);
+ SqlNodeList clusteredBy = null;
+}
+{
+ <DELETE> { s = span(); }
+ <FROM>
+ table = CompoundIdentifier()
+ [
+ <WHERE> source = Expression(ExprContext.ACCEPT_SUB_QUERY)
+ ]
+ [
+ <PARTITIONED> <BY>
+ partitionedBy = PartitionGranularity()
+ ]
+ [
+ <CLUSTERED> <BY>
+ clusteredBy = ClusterItems()
Review Comment:
Since in the backend, Delete triggers a reingest of the datasource, the above is the clustering for that. I think this would be improved with the catalog.
--
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