LakshSingla commented on code in PR #15689:
URL: https://github.com/apache/druid/pull/15689#discussion_r1471114058
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/IngestHandler.java:
##########
@@ -106,12 +110,45 @@ protected String operationName()
@Override
public void validate()
{
- if (ingestNode().getPartitionedBy() == null) {
+ if (ingestNode().getTargetTable() instanceof
ExternalDestinationSqlIdentifier) {
+ if
(!handlerContext.plannerContext().featureAvailable(EngineFeature.WRITE_EXTERNAL_DATA))
{
Review Comment:
Is this validation necessary? `IngestHandler` is only supported via the MSQ
engine, therefore will this message show up? And if so, let's add a test for
the same.
##########
sql/src/main/java/org/apache/druid/sql/destination/ExportDestination.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.
+ */
+
+package org.apache.druid.sql.destination;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonTypeName;
+
+import java.util.Map;
+import java.util.Objects;
+
+/**
+ * Destination that represents an ingestion to an external source.
+ */
+@JsonTypeName(ExportDestination.TYPE_KEY)
+public class ExportDestination implements IngestDestination
+{
+ public static final String TYPE_KEY = "external";
+ private final String destinationType;
+ private final Map<String, String> properties;
+
+ public ExportDestination(@JsonProperty("destinationType") String
destinationType, @JsonProperty("properties") Map<String, String> properties)
+ {
+ this.destinationType = destinationType;
+ this.properties = properties;
+ }
+
+ @JsonProperty("destinationType")
+ public String getDestinationType()
+ {
+ return destinationType;
+ }
+
+ @JsonProperty("properties")
+ public Map<String, String> getProperties()
Review Comment:
I am still not convinced that storing a map of properties is a good idea. I
think that's the limitation that we are working with when we are translating it
from the SQL layer (unless there'd be separate grammar for each thing).
IMO it should be deserialized into `StorageConnectorProvider` when it gets
stored in the `ExportDestination` at the time of creation, not at the time of
application. It's not clear what the properties are supposed to represent
unless you dig into the code, hence my concern. We have that context when we
create this object.
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlIngest.java:
##########
@@ -34,13 +34,18 @@
*/
public abstract class DruidSqlIngest extends SqlInsert
{
+ public static final String SQL_EXPORT_FILE_FORMAT = "__exportFileFormat";
Review Comment:
Is there validation to prevent user from setting up the field? I think we
have some restricted context parameters that we can setup, and perhaps we
should piggyback on that, or add our own validation (if there isn't). Also, a
test to verify that.
##########
sql/src/main/java/org/apache/druid/sql/calcite/run/EngineFeature.java:
##########
@@ -118,5 +118,9 @@ public enum EngineFeature
* and cannot concat the results together (as * the result for broker is the
query id). Therefore, we don't get the
* correct result back, while the MSQ engine is executing the partial query
*/
- ALLOW_TOP_LEVEL_UNION_ALL;
+ ALLOW_TOP_LEVEL_UNION_ALL,
+ /**
+ * Queries can write to an external datasource using {@link
org.apache.druid.sql.destination.ExportDestination}
+ */
+ WRITE_EXTERNAL_DATA;
Review Comment:
minor nit: Can be moved below `READ_EXTERNAL_DATA`.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -1780,6 +1788,11 @@ private static QueryDefinition makeQueryDefinition(
MultiStageQueryContext.getRowsPerPage(querySpec.getQuery().context())
);
queryToPlan = querySpec.getQuery();
+ } else if (querySpec.getDestination() instanceof ExportMSQDestination) {
+ shuffleSpecFactory = ShuffleSpecFactories.getGlobalSortWithTargetSize(
+ MultiStageQueryContext.getRowsPerPage(querySpec.getQuery().context())
+ );
+ queryToPlan = querySpec.getQuery();
Review Comment:
nit: I was wondering if MSQDestination can have a method called
`getShuffleSpecFactory(int targetSize)`, and all the three destinations
implement it. That will get rid of the ugly instanceof checks here.
In case there are no other pressing changes to be made post this review, I
am fine with keeping it the same.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/IngestHandler.java:
##########
@@ -106,12 +110,45 @@ protected String operationName()
@Override
public void validate()
{
- if (ingestNode().getPartitionedBy() == null) {
+ if (ingestNode().getTargetTable() instanceof
ExternalDestinationSqlIdentifier) {
Review Comment:
Refactoring comment: This seems slightly messy, that we are interleaving the
validation for ingest into datasource along with export. It might be easier to
read if the top level validation has following structure:
```java
if (ingestNode.getTargetTable() instanceof ExternalDestinationSqlIdentifier)
{
vallidateExport();
}
else if (ingestNode.getTargetTable() instanceof DataSource(?)) {
validateIngest();
}
else {
throw;
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
