gianm commented on code in PR #15689: URL: https://github.com/apache/druid/pull/15689#discussion_r1475101931
########## sql/src/main/java/org/apache/druid/sql/calcite/parser/ExternalDestinationSqlIdentifier.java: ########## @@ -0,0 +1,114 @@ +/* + * 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.calcite.parser; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.calcite.sql.SqlIdentifier; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlWriter; +import org.apache.calcite.sql.parser.SqlParserPos; +import org.apache.druid.error.DruidException; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.storage.StorageConnectorProvider; +import org.apache.druid.utils.CollectionUtils; + +import java.util.HashMap; +import java.util.Map; + +/** + * Extends the {@link SqlIdentifier} to hold parameters for an external destination. + */ +public class ExternalDestinationSqlIdentifier extends SqlIdentifier +{ + private final Map<String, String> properties; + + public ExternalDestinationSqlIdentifier( + String name, + SqlParserPos pos, + Map<String, String> properties + ) + { + super(name, pos); + this.properties = properties; + } + + public String getDestinationType() Review Comment: Since this is being used for the labels of permissions, please confirm that valid names here line up with the equivalent input source types returned from `InputSource#getTypes`. That way, permission labels will be consistent for reading and writing. ########## extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java: ########## @@ -1872,6 +1871,44 @@ private static QueryDefinition makeQueryDefinition( } else { return queryDef; } + } else if (querySpec.getDestination() instanceof ExportMSQDestination) { + final ExportMSQDestination exportMSQDestination = (ExportMSQDestination) querySpec.getDestination(); + final StorageConnectorProvider storageConnectorProvider = exportMSQDestination.getStorageConnectorProvider(); + + final ResultFormat resultFormat = exportMSQDestination.getResultFormat(); + + // If the statement is a 'REPLACE' statement, delete the existing files at the destination. + if (exportMSQDestination.getReplaceTimeChunks() != null) { + if (Intervals.ONLY_ETERNITY.equals(exportMSQDestination.getReplaceTimeChunks())) { + StorageConnector storageConnector = storageConnectorProvider.get(); + try { + storageConnector.deleteRecursively(""); Review Comment: This worries me because it means that if someone provides a wrong path by accident, we'll delete a bunch of their stuff. It would be better to only delete files that we can confirm came from a previous Druid export. To achieve that we could write our files in a special directory structure, or we could include a manifest file and check for the prior manifest. The manifest file is probably a good idea anyway, since it makes it easier for readers to be sure they got the entire set of files. If we use the Hive "symlink manifest" format in particular, then engines like Presto and Spark will be able to more easily read our exports. Anyway, whatever we do here, I think we should do something different than blindly deleting everything in the path the user provides. It's too likely that it will lead to accidental data deletion. ########## integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/msq/ITMultiStageQuery.java: ########## @@ -176,4 +188,83 @@ public void testMsqIngestionAndQueryingWithLocalFn() throws Exception msqHelper.testQueriesFromFile(QUERY_FILE, datasource); } + + @Test + public void testExport() throws Exception Review Comment: Please add an integration test for the access-denied scenario, to ensure that people without the proper EXTERNAL permission cannot do exports. -- 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]
