Skip to content

Commit c56cdc6

Browse files
alexjo2144alexjo2144
committed
Remove session property overrides for Hive staging location
Users should not have the ability to write files into arbitary locations.
1 parent 7f3bad9 commit c56cdc6

File tree

12 files changed

+47
-114
lines changed

12 files changed

+47
-114
lines changed

‎client/trino-jdbc/src/test/java/io/trino/jdbc/TestJdbcConnection.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -274,16 +274,16 @@ public void testSession()
274274
}
275275

276276
for (String part : ImmutableList.of(",", "=", ":", "|", "/", "\\", "'", "\\'", "''", "\"", "\\\"", "[", "]")) {
277-
String value = format("/tmp/presto-%s-${USER}", part);
277+
String value = format("my-table-%s-name", part);
278278
try {
279279
try (Statement statement = connection.createStatement()) {
280-
statement.execute(format("SET SESSION hive.temporary_staging_directory_path = '%s'", value.replace("'", "''")));
280+
statement.execute(format("SET SESSION spatial_partitioning_table_name = '%s'", value.replace("'", "''")));
281281
}
282282

283283
assertThat(listSession(connection))
284284
.contains("join_distribution_type|BROADCAST|AUTOMATIC")
285285
.contains("exchange_compression|true|false")
286-
.contains(format("hive.temporary_staging_directory_path|%s|/tmp/presto-${USER}", value));
286+
.contains(format("spatial_partitioning_table_name|%s|", value));
287287
}
288288
catch (Exception e) {
289289
fail(format("Failed to set session property value to [%s]", value), e);
@@ -420,14 +420,14 @@ private void testRole(String roleParameterValue, ClientSelectedRole clientSelect
420420
public void testSessionProperties()
421421
throws SQLException
422422
{
423-
try (Connection connection = createConnection("roles=hive:admin&sessionProperties=hive.temporary_staging_directory_path:/tmp;execution_policy:all-at-once")) {
423+
try (Connection connection = createConnection("roles=hive:admin&sessionProperties=hive.hive_views_legacy_translation:true;execution_policy:all-at-once")) {
424424
TrinoConnection trinoConnection = connection.unwrap(TrinoConnection.class);
425425
assertThat(trinoConnection.getSessionProperties())
426-
.extractingByKeys("hive.temporary_staging_directory_path", "execution_policy")
427-
.containsExactly("/tmp", "all-at-once");
426+
.extractingByKeys("hive.hive_views_legacy_translation", "execution_policy")
427+
.containsExactly("true", "all-at-once");
428428
assertThat(listSession(connection)).containsAll(ImmutableSet.of(
429429
"execution_policy|all-at-once|phased",
430-
"hive.temporary_staging_directory_path|/tmp|/tmp/presto-${USER}"));
430+
"hive.hive_views_legacy_translation|true|false"));
431431
}
432432
}
433433

‎plugin/trino-hive-hadoop2/src/test/java/io/trino/plugin/hive/s3select/S3SelectTestHelper.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ public S3SelectTestHelper(String host,
128128
HivePartitionManager hivePartitionManager = new HivePartitionManager(this.hiveConfig);
129129

130130
hdfsEnvironment = new HdfsEnvironment(hdfsConfiguration, new HdfsConfig(), new NoHdfsAuthentication());
131-
locationService = new HiveLocationService(hdfsEnvironment);
131+
locationService = new HiveLocationService(hdfsEnvironment, hiveConfig);
132132
JsonCodec<PartitionUpdate> partitionUpdateCodec = JsonCodec.jsonCodec(PartitionUpdate.class);
133133

134134
metastoreClient = new TestingHiveMetastore(

‎plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveLocationService.java‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
import java.util.Optional;
3030

3131
import static io.trino.plugin.hive.HiveErrorCode.HIVE_PATH_ALREADY_EXISTS;
32-
import static io.trino.plugin.hive.HiveSessionProperties.isTemporaryStagingDirectoryEnabled;
3332
import static io.trino.plugin.hive.LocationHandle.WriteMode.DIRECT_TO_TARGET_EXISTING_DIRECTORY;
3433
import static io.trino.plugin.hive.LocationHandle.WriteMode.DIRECT_TO_TARGET_NEW_DIRECTORY;
3534
import static io.trino.plugin.hive.LocationHandle.WriteMode.STAGE_AND_MOVE_TO_TARGET_DIRECTORY;
@@ -47,11 +46,15 @@ public class HiveLocationService
4746
implements LocationService
4847
{
4948
private final HdfsEnvironment hdfsEnvironment;
49+
private final boolean temporaryStagingDirectoryEnabled;
50+
private final String temporaryStagingDirectoryPath;
5051

5152
@Inject
52-
public HiveLocationService(HdfsEnvironment hdfsEnvironment)
53+
public HiveLocationService(HdfsEnvironment hdfsEnvironment, HiveConfig hiveConfig)
5354
{
5455
this.hdfsEnvironment = requireNonNull(hdfsEnvironment, "hdfsEnvironment is null");
56+
this.temporaryStagingDirectoryEnabled = hiveConfig.isTemporaryStagingDirectoryEnabled();
57+
this.temporaryStagingDirectoryPath = hiveConfig.getTemporaryStagingDirectoryPath();
5558
}
5659

5760
@Override
@@ -79,8 +82,8 @@ public LocationHandle forNewTableAsSelect(SemiTransactionalHiveMetastore metasto
7982
}
8083

8184
// TODO detect when existing table's location is a on a different file system than the temporary directory
82-
if (shouldUseTemporaryDirectory(session, context, new Path(targetPath.toString()), externalLocation.isPresent())) {
83-
Location writePath = createTemporaryPath(session, context, hdfsEnvironment, new Path(targetPath.toString()));
85+
if (shouldUseTemporaryDirectory(context, new Path(targetPath.toString()), externalLocation.isPresent())) {
86+
Location writePath = createTemporaryPath(context, hdfsEnvironment, new Path(targetPath.toString()), temporaryStagingDirectoryPath);
8487
return new LocationHandle(targetPath, writePath, STAGE_AND_MOVE_TO_TARGET_DIRECTORY);
8588
}
8689
return new LocationHandle(targetPath, targetPath, DIRECT_TO_TARGET_NEW_DIRECTORY);
@@ -92,8 +95,8 @@ public LocationHandle forExistingTable(SemiTransactionalHiveMetastore metastore,
9295
HdfsContext context = new HdfsContext(session);
9396
Location targetPath = Location.of(table.getStorage().getLocation());
9497

95-
if (shouldUseTemporaryDirectory(session, context, new Path(targetPath.toString()), false) && !isTransactionalTable(table.getParameters())) {
96-
Location writePath = createTemporaryPath(session, context, hdfsEnvironment, new Path(targetPath.toString()));
98+
if (shouldUseTemporaryDirectory(context, new Path(targetPath.toString()), false) && !isTransactionalTable(table.getParameters())) {
99+
Location writePath = createTemporaryPath(context, hdfsEnvironment, new Path(targetPath.toString()), temporaryStagingDirectoryPath);
97100
return new LocationHandle(targetPath, writePath, STAGE_AND_MOVE_TO_TARGET_DIRECTORY);
98101
}
99102
return new LocationHandle(targetPath, targetPath, DIRECT_TO_TARGET_EXISTING_DIRECTORY);
@@ -107,9 +110,9 @@ public LocationHandle forOptimize(SemiTransactionalHiveMetastore metastore, Conn
107110
return new LocationHandle(targetPath, targetPath, DIRECT_TO_TARGET_EXISTING_DIRECTORY);
108111
}
109112

110-
private boolean shouldUseTemporaryDirectory(ConnectorSession session, HdfsContext context, Path path, boolean hasExternalLocation)
113+
private boolean shouldUseTemporaryDirectory(HdfsContext context, Path path, boolean hasExternalLocation)
111114
{
112-
return isTemporaryStagingDirectoryEnabled(session)
115+
return temporaryStagingDirectoryEnabled
113116
// skip using temporary directory for S3
114117
&& !isS3FileSystem(context, hdfsEnvironment, path)
115118
// skip using temporary directory if destination is encrypted; it's not possible to move a file between encryption zones

‎plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSinkProvider.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,8 @@ public class HivePageSinkProvider
7878
private final HiveWriterStats hiveWriterStats;
7979
private final long perTransactionMetastoreCacheMaximumSize;
8080
private final DateTimeZone parquetTimeZone;
81+
private final boolean temporaryStagingDirectoryDirectoryEnabled;
82+
private final String temporaryStagingDirectoryPath;
8183

8284
@Inject
8385
public HivePageSinkProvider(
@@ -116,6 +118,8 @@ public HivePageSinkProvider(
116118
this.hiveWriterStats = requireNonNull(hiveWriterStats, "hiveWriterStats is null");
117119
this.perTransactionMetastoreCacheMaximumSize = config.getPerTransactionMetastoreCacheMaximumSize();
118120
this.parquetTimeZone = config.getParquetDateTimeZone();
121+
this.temporaryStagingDirectoryDirectoryEnabled = config.isTemporaryStagingDirectoryEnabled();
122+
this.temporaryStagingDirectoryPath = config.getTemporaryStagingDirectoryPath();
119123
}
120124

121125
@Override
@@ -187,7 +191,9 @@ private HivePageSink createPageSink(HiveWritableTableHandle handle, boolean isCr
187191
nodeManager,
188192
eventClient,
189193
hiveSessionProperties,
190-
hiveWriterStats);
194+
hiveWriterStats,
195+
temporaryStagingDirectoryDirectoryEnabled,
196+
temporaryStagingDirectoryPath);
191197

192198
return new HivePageSink(
193199
handle,

‎plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveSessionProperties.java‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,6 @@ public final class HiveSessionProperties
116116
private static final String COLLECT_COLUMN_STATISTICS_ON_WRITE = "collect_column_statistics_on_write";
117117
private static final String OPTIMIZE_MISMATCHED_BUCKET_COUNT = "optimize_mismatched_bucket_count";
118118
private static final String S3_SELECT_PUSHDOWN_ENABLED = "s3_select_pushdown_enabled";
119-
private static final String TEMPORARY_STAGING_DIRECTORY_ENABLED = "temporary_staging_directory_enabled";
120-
private static final String TEMPORARY_STAGING_DIRECTORY_PATH = "temporary_staging_directory_path";
121119
private static final String DELEGATE_TRANSACTIONAL_MANAGED_TABLE_LOCATION_TO_METASTORE = "delegate_transactional_managed_table_location_to_metastore";
122120
private static final String IGNORE_ABSENT_PARTITIONS = "ignore_absent_partitions";
123121
private static final String QUERY_PARTITION_FILTER_REQUIRED = "query_partition_filter_required";
@@ -510,16 +508,6 @@ public HiveSessionProperties(
510508
"S3 Select pushdown enabled",
511509
hiveConfig.isS3SelectPushdownEnabled(),
512510
false),
513-
booleanProperty(
514-
TEMPORARY_STAGING_DIRECTORY_ENABLED,
515-
"Should use temporary staging directory for write operations",
516-
hiveConfig.isTemporaryStagingDirectoryEnabled(),
517-
false),
518-
stringProperty(
519-
TEMPORARY_STAGING_DIRECTORY_PATH,
520-
"Temporary staging directory location",
521-
hiveConfig.getTemporaryStagingDirectoryPath(),
522-
false),
523511
booleanProperty(
524512
DELEGATE_TRANSACTIONAL_MANAGED_TABLE_LOCATION_TO_METASTORE,
525513
"When transactional managed table is created via Trino the location will not be set in request sent to HMS and location will be determined by metastore; if this property is set to true CREATE TABLE AS queries are not supported.",
@@ -950,16 +938,6 @@ public static boolean isOptimizedMismatchedBucketCount(ConnectorSession session)
950938
return session.getProperty(OPTIMIZE_MISMATCHED_BUCKET_COUNT, Boolean.class);
951939
}
952940

953-
public static boolean isTemporaryStagingDirectoryEnabled(ConnectorSession session)
954-
{
955-
return session.getProperty(TEMPORARY_STAGING_DIRECTORY_ENABLED, Boolean.class);
956-
}
957-
958-
public static String getTemporaryStagingDirectoryPath(ConnectorSession session)
959-
{
960-
return session.getProperty(TEMPORARY_STAGING_DIRECTORY_PATH, String.class);
961-
}
962-
963941
public static boolean isDelegateTransactionalManagedTableLocationToMetastore(ConnectorSession session)
964942
{
965943
return session.getProperty(DELEGATE_TRANSACTIONAL_MANAGED_TABLE_LOCATION_TO_METASTORE, Boolean.class);

‎plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveWriterFactory.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,7 @@
8888
import static io.trino.plugin.hive.HiveErrorCode.HIVE_TABLE_READ_ONLY;
8989
import static io.trino.plugin.hive.HiveErrorCode.HIVE_UNSUPPORTED_FORMAT;
9090
import static io.trino.plugin.hive.HiveSessionProperties.getInsertExistingPartitionsBehavior;
91-
import static io.trino.plugin.hive.HiveSessionProperties.getTemporaryStagingDirectoryPath;
9291
import static io.trino.plugin.hive.HiveSessionProperties.getTimestampPrecision;
93-
import static io.trino.plugin.hive.HiveSessionProperties.isTemporaryStagingDirectoryEnabled;
9492
import static io.trino.plugin.hive.HiveType.toHiveType;
9593
import static io.trino.plugin.hive.LocationHandle.WriteMode.DIRECT_TO_TARGET_EXISTING_DIRECTORY;
9694
import static io.trino.plugin.hive.acid.AcidOperation.CREATE_TABLE;
@@ -199,7 +197,9 @@ public HiveWriterFactory(
199197
NodeManager nodeManager,
200198
EventClient eventClient,
201199
HiveSessionProperties hiveSessionProperties,
202-
HiveWriterStats hiveWriterStats)
200+
HiveWriterStats hiveWriterStats,
201+
boolean sortedWritingTempStagingPathEnabled,
202+
String sortedWritingTempStagingPath)
203203
{
204204
this.fileWriterFactories = ImmutableSet.copyOf(requireNonNull(fileWriterFactories, "fileWriterFactories is null"));
205205
this.fileSystem = fileSystemFactory.create(session);
@@ -221,8 +221,8 @@ public HiveWriterFactory(
221221
this.pageSorter = requireNonNull(pageSorter, "pageSorter is null");
222222
this.sortBufferSize = requireNonNull(sortBufferSize, "sortBufferSize is null");
223223
this.maxOpenSortFiles = maxOpenSortFiles;
224-
this.sortedWritingTempStagingPathEnabled = isTemporaryStagingDirectoryEnabled(session);
225-
this.sortedWritingTempStagingPath = getTemporaryStagingDirectoryPath(session);
224+
this.sortedWritingTempStagingPathEnabled = sortedWritingTempStagingPathEnabled;
225+
this.sortedWritingTempStagingPath = requireNonNull(sortedWritingTempStagingPath, "sortedWritingTempStagingPath is null");
226226
this.insertExistingPartitionsBehavior = getInsertExistingPartitionsBehavior(session);
227227
this.parquetTimeZone = requireNonNull(parquetTimeZone, "parquetTimeZone is null");
228228

‎plugin/trino-hive/src/main/java/io/trino/plugin/hive/util/HiveWriteUtils.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,6 @@
9797
import static io.trino.plugin.hive.HiveErrorCode.HIVE_SERDE_NOT_FOUND;
9898
import static io.trino.plugin.hive.HiveErrorCode.HIVE_WRITER_DATA_ERROR;
9999
import static io.trino.plugin.hive.HivePartitionKey.HIVE_DEFAULT_DYNAMIC_PARTITION;
100-
import static io.trino.plugin.hive.HiveSessionProperties.getTemporaryStagingDirectoryPath;
101100
import static io.trino.plugin.hive.TableType.MANAGED_TABLE;
102101
import static io.trino.plugin.hive.TableType.MATERIALIZED_VIEW;
103102
import static io.trino.plugin.hive.metastore.MetastoreUtil.getProtectMode;
@@ -516,11 +515,10 @@ public static boolean isFileCreatedByQuery(String fileName, String queryId)
516515
return fileName.startsWith(queryId) || fileName.endsWith(queryId);
517516
}
518517

519-
public static Location createTemporaryPath(ConnectorSession session, HdfsContext context, HdfsEnvironment hdfsEnvironment, Path targetPath)
518+
public static Location createTemporaryPath(HdfsContext context, HdfsEnvironment hdfsEnvironment, Path targetPath, String temporaryStagingDirectoryPath)
520519
{
521520
// use a per-user temporary directory to avoid permission problems
522-
String temporaryPrefix = getTemporaryStagingDirectoryPath(session)
523-
.replace("${USER}", context.getIdentity().getUser());
521+
String temporaryPrefix = temporaryStagingDirectoryPath.replace("${USER}", context.getIdentity().getUser());
524522

525523
// use relative temporary directory on ViewFS
526524
if (isViewFileSystem(context, hdfsEnvironment, targetPath)) {

‎plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -221,8 +221,6 @@
221221
import static io.trino.plugin.hive.HiveErrorCode.HIVE_PARTITION_SCHEMA_MISMATCH;
222222
import static io.trino.plugin.hive.HiveMetadata.PRESTO_QUERY_ID_NAME;
223223
import static io.trino.plugin.hive.HiveMetadata.PRESTO_VERSION_NAME;
224-
import static io.trino.plugin.hive.HiveSessionProperties.getTemporaryStagingDirectoryPath;
225-
import static io.trino.plugin.hive.HiveSessionProperties.isTemporaryStagingDirectoryEnabled;
226224
import static io.trino.plugin.hive.HiveStorageFormat.AVRO;
227225
import static io.trino.plugin.hive.HiveStorageFormat.CSV;
228226
import static io.trino.plugin.hive.HiveStorageFormat.JSON;
@@ -825,7 +823,7 @@ protected final void setup(String databaseName, HiveConfig hiveConfig, HiveMetas
825823
metastoreClient = hiveMetastore;
826824
hdfsEnvironment = hdfsConfiguration;
827825
HivePartitionManager partitionManager = new HivePartitionManager(hiveConfig);
828-
locationService = new HiveLocationService(hdfsEnvironment);
826+
locationService = new HiveLocationService(hdfsEnvironment, hiveConfig);
829827
JsonCodec<PartitionUpdate> partitionUpdateCodec = JsonCodec.jsonCodec(PartitionUpdate.class);
830828
countingDirectoryLister = new CountingDirectoryLister();
831829
metadataFactory = new HiveMetadataFactory(
@@ -945,7 +943,7 @@ public Optional<ConnectorMaterializedViewDefinition> getMaterializedView(Connect
945943
protected HiveConfig getHiveConfig()
946944
{
947945
return new HiveConfig()
948-
.setTemporaryStagingDirectoryPath(temporaryStagingDirectory.toAbsolutePath().toString());
946+
.setTemporaryStagingDirectoryPath(temporaryStagingDirectory.resolve("temp_path_").toAbsolutePath().toString());
949947
}
950948

951949
protected SortingFileWriterConfig getSortingFileWriterConfig()
@@ -2857,16 +2855,16 @@ private void doTestBucketSortedTables(SchemaTableName table)
28572855
}
28582856

28592857
HdfsContext context = new HdfsContext(session);
2858+
HiveConfig config = getHiveConfig();
28602859
// verify we have enough temporary files per bucket to require multiple passes
28612860
Location stagingPathRoot;
2862-
if (isTemporaryStagingDirectoryEnabled(session)) {
2863-
stagingPathRoot = Location.of(getTemporaryStagingDirectoryPath(session)
2861+
if (config.isTemporaryStagingDirectoryEnabled()) {
2862+
stagingPathRoot = Location.of(config.getTemporaryStagingDirectoryPath()
28642863
.replace("${USER}", context.getIdentity().getUser()));
28652864
}
28662865
else {
28672866
stagingPathRoot = getStagingPathRoot(outputHandle);
28682867
}
2869-
28702868
assertThat(listAllDataFiles(context, stagingPathRoot))
28712869
.filteredOn(file -> file.contains(".tmp-sort."))
28722870
.size().isGreaterThan(bucketCount * getSortingFileWriterConfig().getMaxOpenSortFiles() * 2);
@@ -3075,12 +3073,15 @@ public void testCreateEmptyTableShouldNotCreateStagingDirectory()
30753073
try {
30763074
List<Column> columns = ImmutableList.of(new Column("test", HIVE_STRING, Optional.empty()));
30773075
try (Transaction transaction = newTransaction()) {
3078-
final String temporaryStagingPrefix = "hive-temporary-staging-prefix-" + UUID.randomUUID().toString().toLowerCase(ENGLISH).replace("-", "");
3079-
ConnectorSession session = newSession(ImmutableMap.of("hive.temporary_staging_directory_path", temporaryStagingPrefix));
3076+
String temporaryStagingPrefix = "hive-temporary-staging-prefix-" + UUID.randomUUID().toString().toLowerCase(ENGLISH).replace("-", "");
3077+
ConnectorSession session = newSession();
30803078
String tableOwner = session.getUser();
30813079
String schemaName = temporaryCreateEmptyTable.getSchemaName();
30823080
String tableName = temporaryCreateEmptyTable.getTableName();
3083-
LocationService locationService = getLocationService();
3081+
HiveConfig hiveConfig = getHiveConfig()
3082+
.setTemporaryStagingDirectoryPath(temporaryStagingPrefix)
3083+
.setTemporaryStagingDirectoryEnabled(true);
3084+
LocationService locationService = new HiveLocationService(hdfsEnvironment, hiveConfig);
30843085
Location targetPath = locationService.forNewTable(transaction.getMetastore(), session, schemaName, tableName);
30853086
Table.Builder tableBuilder = Table.builder()
30863087
.setDatabaseName(schemaName)

‎plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHiveFileSystem.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ protected void setup(String host, int port, String databaseName, boolean s3Selec
200200
.build()),
201201
getBasePath(),
202202
hdfsEnvironment);
203-
locationService = new HiveLocationService(hdfsEnvironment);
203+
locationService = new HiveLocationService(hdfsEnvironment, config);
204204
JsonCodec<PartitionUpdate> partitionUpdateCodec = JsonCodec.jsonCodec(PartitionUpdate.class);
205205
metadataFactory = new HiveMetadataFactory(
206206
new CatalogName("hive"),

0 commit comments

Comments
 (0)