diff --git a/docs/src/main/sphinx/connector/iceberg.rst b/docs/src/main/sphinx/connector/iceberg.rst index cea1813c8f36..1e4e73215d7a 100644 --- a/docs/src/main/sphinx/connector/iceberg.rst +++ b/docs/src/main/sphinx/connector/iceberg.rst @@ -403,6 +403,7 @@ TABLE ` syntax. Optionally specify the WITH ( format = 'PARQUET', partitioning = ARRAY['c1', 'c2'], + sorted_by = ARRAY['c3'], location = 's3://my-bucket/a/path/' ); @@ -571,6 +572,7 @@ The following table properties can be updated after a table is created: * ``format`` * ``format_version`` * ``partitioning`` +* ``sorted_by`` For example, to update a table from v1 of the Iceberg specification to v2: @@ -793,6 +795,46 @@ In this example, the table is partitioned by the month of ``order_date``, a hash country VARCHAR) WITH (partitioning = ARRAY['month(order_date)', 'bucket(account_number, 10)', 'country']) +Sorted tables +------------- + +The connector supports sorted files as a performance improvement. Data is +sorted during writes within each file based on the specified array of one +or more columns. + +Sorting is particularly beneficial when the sorted columns show a +high cardinality and are used as a filter for selective reads. + +The sort order is configured with the ``sorted_by`` table property. +Specify an array of one or more columns to use for sorting +when creating the table. The following example configures the +``order_date`` column of the ``orders`` table in the ``customers`` +schema in the ``example`` catalog:: + + CREATE TABLE example.customers.orders ( + order_id BIGINT, + order_date DATE, + account_number BIGINT, + customer VARCHAR, + country VARCHAR) + WITH (sorted_by = ARRAY['order_date']) + +Sorting can be combined with partitioning on the same column. For example:: + + CREATE TABLE example.customers.orders ( + order_id BIGINT, + order_date DATE, + account_number BIGINT, + customer VARCHAR, + country VARCHAR) + WITH ( + partitioning = ARRAY['month(order_date)'], + sorted_by = ARRAY['order_date'] + ) + +You can disable sorted writing with the session property +``sorted_writing_enabled`` set to ``false``. + Rolling back to a previous snapshot ----------------------------------- diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java index 1639e524190b..dc5995b0dd5a 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java @@ -31,7 +31,6 @@ import javax.validation.constraints.AssertTrue; import javax.validation.constraints.DecimalMax; import javax.validation.constraints.DecimalMin; -import javax.validation.constraints.Max; import javax.validation.constraints.Min; import javax.validation.constraints.NotNull; @@ -82,7 +81,6 @@ public class HiveConfig private Integer maxSplitsPerSecond; private DataSize maxInitialSplitSize; private int domainCompactionThreshold = 100; - private DataSize writerSortBufferSize = DataSize.of(64, MEGABYTE); private boolean forceLocalScheduling; private boolean recursiveDirWalkerEnabled; private boolean ignoreAbsentPartitions; @@ -105,7 +103,6 @@ public class HiveConfig // to avoid deleting those files if Trino is unable to check. private boolean deleteSchemaLocationsFallback; private int maxPartitionsPerWriter = 100; - private int maxOpenSortFiles = 50; private int writeValidationThreads = 16; private boolean validateBucketing = true; private boolean parallelPartitionedBucketedWrites = true; @@ -274,20 +271,6 @@ public HiveConfig setTargetMaxFileSize(DataSize targetMaxFileSize) return this; } - @MinDataSize("1MB") - @MaxDataSize("1GB") - public DataSize getWriterSortBufferSize() - { - return writerSortBufferSize; - } - - @Config("hive.writer-sort-buffer-size") - public HiveConfig setWriterSortBufferSize(DataSize writerSortBufferSize) - { - this.writerSortBufferSize = writerSortBufferSize; - return this; - } - public boolean isForceLocalScheduling() { return forceLocalScheduling; @@ -609,21 +592,6 @@ public HiveConfig setMaxPartitionsPerWriter(int maxPartitionsPerWriter) return this; } - @Min(2) - @Max(1000) - public int getMaxOpenSortFiles() - { - return maxOpenSortFiles; - } - - @Config("hive.max-open-sort-files") - @ConfigDescription("Maximum number of writer temporary files to read in one pass") - public HiveConfig setMaxOpenSortFiles(int maxOpenSortFiles) - { - this.maxOpenSortFiles = maxOpenSortFiles; - return this; - } - public int getWriteValidationThreads() { return writeValidationThreads; diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveModule.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveModule.java index b7b648165a20..3d203431b9a0 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveModule.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveModule.java @@ -76,6 +76,7 @@ public void configure(Binder binder) { configBinder(binder).bindConfig(HiveConfig.class); configBinder(binder).bindConfig(HiveMetastoreConfig.class); + configBinder(binder).bindConfig(SortingFileWriterConfig.class, "hive"); binder.bind(HiveSessionProperties.class).in(Scopes.SINGLETON); binder.bind(HiveTableProperties.class).in(Scopes.SINGLETON); diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSinkProvider.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSinkProvider.java index e2fdfe255c64..6c0ee6dc19c1 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSinkProvider.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSinkProvider.java @@ -89,6 +89,7 @@ public HivePageSinkProvider( PageIndexerFactory pageIndexerFactory, TypeManager typeManager, HiveConfig config, + SortingFileWriterConfig sortingFileWriterConfig, LocationService locationService, JsonCodec partitionUpdateCodec, NodeManager nodeManager, @@ -104,8 +105,8 @@ public HivePageSinkProvider( this.pageIndexerFactory = requireNonNull(pageIndexerFactory, "pageIndexerFactory is null"); this.typeManager = requireNonNull(typeManager, "typeManager is null"); this.maxOpenPartitions = config.getMaxPartitionsPerWriter(); - this.maxOpenSortFiles = config.getMaxOpenSortFiles(); - this.writerSortBufferSize = requireNonNull(config.getWriterSortBufferSize(), "writerSortBufferSize is null"); + this.maxOpenSortFiles = sortingFileWriterConfig.getMaxOpenSortFiles(); + this.writerSortBufferSize = requireNonNull(sortingFileWriterConfig.getWriterSortBufferSize(), "writerSortBufferSize is null"); this.locationService = requireNonNull(locationService, "locationService is null"); this.writeVerificationExecutor = listeningDecorator(newFixedThreadPool(config.getWriteValidationThreads(), daemonThreadsNamed("hive-write-validation-%s"))); this.partitionUpdateCodec = requireNonNull(partitionUpdateCodec, "partitionUpdateCodec is null"); diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/SortingFileWriterConfig.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/SortingFileWriterConfig.java new file mode 100644 index 000000000000..e6f6608c34ea --- /dev/null +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/SortingFileWriterConfig.java @@ -0,0 +1,60 @@ +/* + * Licensed 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 io.trino.plugin.hive; + +import io.airlift.configuration.Config; +import io.airlift.configuration.ConfigDescription; +import io.airlift.units.DataSize; +import io.airlift.units.MaxDataSize; +import io.airlift.units.MinDataSize; + +import javax.validation.constraints.Max; +import javax.validation.constraints.Min; + +import static io.airlift.units.DataSize.Unit.MEGABYTE; + +public class SortingFileWriterConfig +{ + private DataSize writerSortBufferSize = DataSize.of(64, MEGABYTE); + private int maxOpenSortFiles = 50; + + @MinDataSize("1MB") + @MaxDataSize("1GB") + public DataSize getWriterSortBufferSize() + { + return writerSortBufferSize; + } + + @Config("writer-sort-buffer-size") + public SortingFileWriterConfig setWriterSortBufferSize(DataSize writerSortBufferSize) + { + this.writerSortBufferSize = writerSortBufferSize; + return this; + } + + @Min(2) + @Max(1000) + public int getMaxOpenSortFiles() + { + return maxOpenSortFiles; + } + + @Config("max-open-sort-files") + @ConfigDescription("Maximum number of writer temporary files to read in one pass") + public SortingFileWriterConfig setMaxOpenSortFiles(int maxOpenSortFiles) + { + this.maxOpenSortFiles = maxOpenSortFiles; + return this; + } +} diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java index 4db4b789d389..9a83a54f8734 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java @@ -910,6 +910,7 @@ public Optional getMaterializedView(Connect new GroupByHashPageIndexerFactory(JOIN_COMPILER, BLOCK_TYPE_OPERATORS), TESTING_TYPE_MANAGER, getHiveConfig(), + getSortingFileWriterConfig(), locationService, partitionUpdateCodec, new TestingNodeManager("fake-environment"), @@ -934,8 +935,13 @@ public Optional getMaterializedView(Connect protected HiveConfig getHiveConfig() { return new HiveConfig() + .setTemporaryStagingDirectoryPath(temporaryStagingDirectory.toAbsolutePath().toString()); + } + + protected SortingFileWriterConfig getSortingFileWriterConfig() + { + return new SortingFileWriterConfig() .setMaxOpenSortFiles(10) - .setTemporaryStagingDirectoryPath(temporaryStagingDirectory.toAbsolutePath().toString()) .setWriterSortBufferSize(DataSize.of(100, KILOBYTE)); } @@ -2764,7 +2770,7 @@ private void doTestBucketSortedTables(SchemaTableName table) assertThat(listAllDataFiles(context, stagingPathRoot)) .filteredOn(file -> file.contains(".tmp-sort.")) - .size().isGreaterThan(bucketCount * getHiveConfig().getMaxOpenSortFiles() * 2); + .size().isGreaterThan(bucketCount * getSortingFileWriterConfig().getMaxOpenSortFiles() * 2); // finish the write Collection fragments = getFutureValue(sink.finish()); diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHiveFileSystem.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHiveFileSystem.java index 9f17bce3e35f..bd01a7c88a31 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHiveFileSystem.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHiveFileSystem.java @@ -249,6 +249,7 @@ protected void setup(String host, int port, String databaseName, boolean s3Selec new GroupByHashPageIndexerFactory(new JoinCompiler(typeOperators), blockTypeOperators), TESTING_TYPE_MANAGER, config, + new SortingFileWriterConfig(), locationService, partitionUpdateCodec, new TestingNodeManager("fake-environment"), diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConfig.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConfig.java index 04f31732983b..cbf5ec738ac9 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConfig.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConfig.java @@ -53,7 +53,6 @@ public void testDefaults() .setMaxSplitsPerSecond(null) .setDomainCompactionThreshold(100) .setTargetMaxFileSize(DataSize.of(1, Unit.GIGABYTE)) - .setWriterSortBufferSize(DataSize.of(64, Unit.MEGABYTE)) .setForceLocalScheduling(false) .setMaxConcurrentFileSystemOperations(20) .setMaxConcurrentMetastoreDrops(20) @@ -71,7 +70,6 @@ public void testDefaults() .setSortedWritingEnabled(true) .setPropagateTableScanSortingProperties(false) .setMaxPartitionsPerWriter(100) - .setMaxOpenSortFiles(50) .setWriteValidationThreads(16) .setValidateBucketing(true) .setParallelPartitionedBucketedWrites(true) @@ -141,7 +139,6 @@ public void testExplicitPropertyMappings() .put("hive.max-splits-per-second", "1") .put("hive.domain-compaction-threshold", "42") .put("hive.target-max-file-size", "72MB") - .put("hive.writer-sort-buffer-size", "13MB") .put("hive.recursive-directories", "true") .put("hive.ignore-absent-partitions", "true") .put("hive.storage-format", "SEQUENCEFILE") @@ -152,7 +149,6 @@ public void testExplicitPropertyMappings() .put("hive.create-empty-bucket-files", "true") .put("hive.delete-schema-locations-fallback", "true") .put("hive.max-partitions-per-writers", "222") - .put("hive.max-open-sort-files", "333") .put("hive.write-validation-threads", "11") .put("hive.validate-bucketing", "false") .put("hive.parallel-partitioned-bucketed-writes", "false") @@ -226,7 +222,6 @@ public void testExplicitPropertyMappings() .setMaxSplitsPerSecond(1) .setDomainCompactionThreshold(42) .setTargetMaxFileSize(DataSize.of(72, Unit.MEGABYTE)) - .setWriterSortBufferSize(DataSize.of(13, Unit.MEGABYTE)) .setForceLocalScheduling(true) .setMaxConcurrentFileSystemOperations(100) .setMaxConcurrentMetastoreDrops(100) @@ -242,7 +237,6 @@ public void testExplicitPropertyMappings() .setCreateEmptyBucketFiles(true) .setDeleteSchemaLocationsFallback(true) .setMaxPartitionsPerWriter(222) - .setMaxOpenSortFiles(333) .setWriteValidationThreads(11) .setValidateBucketing(false) .setParallelPartitionedBucketedWrites(false) diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHivePageSink.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHivePageSink.java index b0a1ab0bf899..8187df31963f 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHivePageSink.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHivePageSink.java @@ -108,6 +108,7 @@ public void testAllFormats() throws Exception { HiveConfig config = new HiveConfig(); + SortingFileWriterConfig sortingFileWriterConfig = new SortingFileWriterConfig(); File tempDir = Files.createTempDirectory(null).toFile(); try { HiveMetastore metastore = createTestingFileHiveMetastore(new File(tempDir, "metastore")); @@ -122,7 +123,7 @@ public void testAllFormats() } config.setHiveStorageFormat(format); config.setHiveCompressionCodec(NONE); - long uncompressedLength = writeTestFile(config, metastore, makeFileName(tempDir, config)); + long uncompressedLength = writeTestFile(config, sortingFileWriterConfig, metastore, makeFileName(tempDir, config)); assertGreaterThan(uncompressedLength, 0L); for (HiveCompressionOption codec : HiveCompressionOption.values()) { @@ -132,12 +133,12 @@ public void testAllFormats() config.setHiveCompressionCodec(codec); if (!isSupportedCodec(format, codec)) { - assertThatThrownBy(() -> writeTestFile(config, metastore, makeFileName(tempDir, config))) + assertThatThrownBy(() -> writeTestFile(config, sortingFileWriterConfig, metastore, makeFileName(tempDir, config))) .hasMessage("Compression codec " + codec + " not supported for " + format); continue; } - long length = writeTestFile(config, metastore, makeFileName(tempDir, config)); + long length = writeTestFile(config, sortingFileWriterConfig, metastore, makeFileName(tempDir, config)); assertTrue(uncompressedLength > length, format("%s with %s compressed to %s which is not less than %s", format, codec, length, uncompressedLength)); } } @@ -160,11 +161,11 @@ private static String makeFileName(File tempDir, HiveConfig config) return tempDir.getAbsolutePath() + "/" + config.getHiveStorageFormat().name() + "." + config.getHiveCompressionCodec().name(); } - private static long writeTestFile(HiveConfig config, HiveMetastore metastore, String outputPath) + private static long writeTestFile(HiveConfig config, SortingFileWriterConfig sortingFileWriterConfig, HiveMetastore metastore, String outputPath) { HiveTransactionHandle transaction = new HiveTransactionHandle(false); HiveWriterStats stats = new HiveWriterStats(); - ConnectorPageSink pageSink = createPageSink(transaction, config, metastore, new Path("file:///" + outputPath), stats); + ConnectorPageSink pageSink = createPageSink(transaction, config, sortingFileWriterConfig, metastore, new Path("file:///" + outputPath), stats); List columns = getTestColumns(); List columnTypes = columns.stream() .map(LineItemColumn::getType) @@ -280,7 +281,7 @@ private static ConnectorPageSource createPageSource(HiveTransactionHandle transa return provider.createPageSource(transaction, getHiveSession(config), split, table, ImmutableList.copyOf(getColumnHandles()), DynamicFilter.EMPTY); } - private static ConnectorPageSink createPageSink(HiveTransactionHandle transaction, HiveConfig config, HiveMetastore metastore, Path outputPath, HiveWriterStats stats) + private static ConnectorPageSink createPageSink(HiveTransactionHandle transaction, HiveConfig config, SortingFileWriterConfig sortingFileWriterConfig, HiveMetastore metastore, Path outputPath, HiveWriterStats stats) { LocationHandle locationHandle = new LocationHandle(outputPath, outputPath, DIRECT_TO_TARGET_NEW_DIRECTORY); HiveOutputTableHandle handle = new HiveOutputTableHandle( @@ -310,6 +311,7 @@ private static ConnectorPageSink createPageSink(HiveTransactionHandle transactio new GroupByHashPageIndexerFactory(new JoinCompiler(typeOperators), blockTypeOperators), TESTING_TYPE_MANAGER, config, + sortingFileWriterConfig, new HiveLocationService(HDFS_ENVIRONMENT), partitionUpdateCodec, new TestingNodeManager("fake-environment"), diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestSortingFileWriterConfig.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestSortingFileWriterConfig.java new file mode 100644 index 000000000000..09aa6ae02e85 --- /dev/null +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestSortingFileWriterConfig.java @@ -0,0 +1,50 @@ +/* + * Licensed 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 io.trino.plugin.hive; + +import com.google.common.collect.ImmutableMap; +import io.airlift.units.DataSize; +import org.testng.annotations.Test; + +import java.util.Map; + +import static io.airlift.configuration.testing.ConfigAssertions.assertFullMapping; +import static io.airlift.configuration.testing.ConfigAssertions.assertRecordedDefaults; +import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults; +import static io.airlift.units.DataSize.Unit.GIGABYTE; +import static io.airlift.units.DataSize.Unit.MEGABYTE; + +public class TestSortingFileWriterConfig +{ + @Test + public void testDefaults() + { + assertRecordedDefaults(recordDefaults(SortingFileWriterConfig.class) + .setWriterSortBufferSize(DataSize.of(64, MEGABYTE)) + .setMaxOpenSortFiles(50)); + } + + @Test + public void testExplicitPropertyMappings() + { + Map properties = ImmutableMap.builder() + .put("writer-sort-buffer-size", "1GB") + .put("max-open-sort-files", "3") + .buildOrThrow(); + SortingFileWriterConfig expected = new SortingFileWriterConfig() + .setWriterSortBufferSize(DataSize.of(1, GIGABYTE)) + .setMaxOpenSortFiles(3); + assertFullMapping(properties, expected); + } +} diff --git a/plugin/trino-iceberg/pom.xml b/plugin/trino-iceberg/pom.xml index 95126348eaee..4dffe2390afb 100644 --- a/plugin/trino-iceberg/pom.xml +++ b/plugin/trino-iceberg/pom.xml @@ -491,6 +491,11 @@ + + org.antlr + antlr4-maven-plugin + + org.apache.maven.plugins maven-enforcer-plugin diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java index fa63fe44469b..9f0c6d0a3c4e 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java @@ -71,6 +71,7 @@ public class IcebergConfig private boolean deleteSchemaLocationsFallback; private double minimumAssignedSplitWeight = 0.05; private Optional materializedViewsStorageSchema = Optional.empty(); + private boolean sortedWritingEnabled = true; public CatalogType getCatalogType() { @@ -339,4 +340,17 @@ public IcebergConfig setMaterializedViewsStorageSchema(String materializedViewsS this.materializedViewsStorageSchema = Optional.ofNullable(materializedViewsStorageSchema); return this; } + + public boolean isSortedWritingEnabled() + { + return sortedWritingEnabled; + } + + @Config("iceberg.sorted-writing-enabled") + @ConfigDescription("Enable sorted writing to tables with a specified sort order") + public IcebergConfig setSortedWritingEnabled(boolean sortedWritingEnabled) + { + this.sortedWritingEnabled = sortedWritingEnabled; + return this; + } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java index 52edacfbb1df..2f4e02c5cb78 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java @@ -115,11 +115,14 @@ import org.apache.iceberg.PartitionField; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.PartitionSpecParser; +import org.apache.iceberg.ReplaceSortOrder; import org.apache.iceberg.RewriteFiles; import org.apache.iceberg.RowDelta; import org.apache.iceberg.Schema; import org.apache.iceberg.SchemaParser; import org.apache.iceberg.Snapshot; +import org.apache.iceberg.SortField; +import org.apache.iceberg.SortOrder; import org.apache.iceberg.StatisticsFile; import org.apache.iceberg.Table; import org.apache.iceberg.TableProperties; @@ -205,6 +208,7 @@ import static io.trino.plugin.iceberg.IcebergTableProperties.FILE_FORMAT_PROPERTY; import static io.trino.plugin.iceberg.IcebergTableProperties.FORMAT_VERSION_PROPERTY; import static io.trino.plugin.iceberg.IcebergTableProperties.PARTITIONING_PROPERTY; +import static io.trino.plugin.iceberg.IcebergTableProperties.SORTED_BY_PROPERTY; import static io.trino.plugin.iceberg.IcebergTableProperties.getPartitioning; import static io.trino.plugin.iceberg.IcebergUtil.canEnforceColumnConstraintInSpecs; import static io.trino.plugin.iceberg.IcebergUtil.commit; @@ -221,6 +225,7 @@ import static io.trino.plugin.iceberg.IcebergUtil.schemaFromMetadata; import static io.trino.plugin.iceberg.PartitionFields.parsePartitionFields; import static io.trino.plugin.iceberg.PartitionFields.toPartitionFields; +import static io.trino.plugin.iceberg.SortFieldUtils.parseSortFields; import static io.trino.plugin.iceberg.TableStatisticsReader.TRINO_STATS_COLUMN_ID_PATTERN; import static io.trino.plugin.iceberg.TableStatisticsReader.TRINO_STATS_PREFIX; import static io.trino.plugin.iceberg.TableType.DATA; @@ -270,7 +275,7 @@ public class IcebergMetadata private static final int CLEANING_UP_PROCEDURES_MAX_SUPPORTED_TABLE_VERSION = 2; private static final String RETENTION_THRESHOLD = "retention_threshold"; private static final String UNKNOWN_SNAPSHOT_TOKEN = "UNKNOWN"; - public static final Set UPDATABLE_TABLE_PROPERTIES = ImmutableSet.of(FILE_FORMAT_PROPERTY, FORMAT_VERSION_PROPERTY, PARTITIONING_PROPERTY); + public static final Set UPDATABLE_TABLE_PROPERTIES = ImmutableSet.of(FILE_FORMAT_PROPERTY, FORMAT_VERSION_PROPERTY, PARTITIONING_PROPERTY, SORTED_BY_PROPERTY); public static final String ORC_BLOOM_FILTER_COLUMNS_KEY = "orc.bloom.filter.columns"; public static final String ORC_BLOOM_FILTER_FPP_KEY = "orc.bloom.filter.fpp"; @@ -382,6 +387,9 @@ public IcebergTableHandle getTableHandle( name.getTableType(), tableSnapshotId, SchemaParser.toJson(tableSchema), + table.sortOrder().fields().stream() + .map(TrinoSortField::fromIceberg) + .collect(toImmutableList()), partitionSpec.map(PartitionSpecParser::toJson), table.operations().current().formatVersion(), TupleDomain.all(), @@ -795,6 +803,7 @@ private IcebergWritableTableHandle newWritableTableHandle(SchemaTableName name, SchemaParser.toJson(table.schema()), transformValues(table.specs(), PartitionSpecParser::toJson), table.spec().specId(), + getSupportedSortFields(table.schema(), table.sortOrder()), getColumns(table.schema(), typeManager), table.location(), getFileFormat(table), @@ -802,6 +811,30 @@ private IcebergWritableTableHandle newWritableTableHandle(SchemaTableName name, retryMode); } + private static List getSupportedSortFields(Schema schema, SortOrder sortOrder) + { + if (!sortOrder.isSorted()) { + return ImmutableList.of(); + } + Set baseColumnFieldIds = schema.columns().stream() + .map(Types.NestedField::fieldId) + .collect(toImmutableSet()); + + ImmutableList.Builder sortFields = ImmutableList.builder(); + for (SortField sortField : sortOrder.fields()) { + if (!sortField.transform().isIdentity()) { + continue; + } + if (!baseColumnFieldIds.contains(sortField.sourceId())) { + continue; + } + + sortFields.add(TrinoSortField.fromIceberg(sortField)); + } + + return sortFields.build(); + } + @Override public Optional finishInsert(ConnectorSession session, ConnectorInsertTableHandle insertHandle, Collection fragments, Collection computedStatistics) { @@ -982,6 +1015,7 @@ private Optional getTableHandleForOptimize(IcebergT tableHandle.getTableSchemaJson(), tableHandle.getPartitionSpecJson().orElseThrow(() -> new VerifyException("Partition spec missing in the table handle")), getColumns(SchemaParser.fromJson(tableHandle.getTableSchemaJson()), typeManager), + tableHandle.getSortOrder(), getFileFormat(tableHandle.getStorageProperties()), tableHandle.getStorageProperties(), maxScannedFileSize, @@ -1468,6 +1502,20 @@ public void setTableProperties(ConnectorSession session, ConnectorTableHandle ta updatePartitioning(icebergTable, transaction, partitionColumns); } + if (properties.containsKey(SORTED_BY_PROPERTY)) { + @SuppressWarnings("unchecked") + List sortColumns = (List) properties.get(SORTED_BY_PROPERTY) + .orElseThrow(() -> new IllegalArgumentException("The sorted_by property cannot be empty")); + ReplaceSortOrder replaceSortOrder = transaction.replaceSortOrder(); + parseSortFields(replaceSortOrder, sortColumns); + try { + replaceSortOrder.commit(); + } + catch (RuntimeException e) { + throw new TrinoException(ICEBERG_COMMIT_ERROR, "Failed to set the sorted_by property", e); + } + } + try { transaction.commitTransaction(); } @@ -2203,6 +2251,7 @@ else if (isMetadataColumnId(columnHandle.getId())) { table.getTableType(), table.getSnapshotId(), table.getTableSchemaJson(), + table.getSortOrder(), table.getPartitionSpecJson(), table.getFormatVersion(), newUnenforcedConstraint, @@ -2352,6 +2401,7 @@ public TableStatistics getTableStatistics(ConnectorSession session, ConnectorTab originalHandle.getTableType(), originalHandle.getSnapshotId(), originalHandle.getTableSchemaJson(), + originalHandle.getSortOrder(), originalHandle.getPartitionSpecJson(), originalHandle.getFormatVersion(), originalHandle.getUnenforcedPredicate(), diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java index 2d965bc45677..8b9a48f6a721 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java @@ -20,6 +20,7 @@ import com.google.inject.multibindings.Multibinder; import io.trino.plugin.base.session.SessionPropertiesProvider; import io.trino.plugin.hive.FileFormatDataSourceStats; +import io.trino.plugin.hive.SortingFileWriterConfig; import io.trino.plugin.hive.metastore.thrift.TranslateHiveViews; import io.trino.plugin.hive.orc.OrcReaderConfig; import io.trino.plugin.hive.orc.OrcWriterConfig; @@ -53,6 +54,7 @@ public void configure(Binder binder) binder.bind(IcebergTransactionManager.class).in(Scopes.SINGLETON); binder.bind(Key.get(boolean.class, TranslateHiveViews.class)).toInstance(false); configBinder(binder).bindConfig(IcebergConfig.class); + configBinder(binder).bindConfig(SortingFileWriterConfig.class, "iceberg"); newSetBinder(binder, SessionPropertiesProvider.class).addBinding().to(IcebergSessionProperties.class).in(Scopes.SINGLETON); binder.bind(IcebergTableProperties.class).in(Scopes.SINGLETON); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSink.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSink.java index 5bc2df10abf6..d1a45fd91a60 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSink.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSink.java @@ -17,15 +17,18 @@ import com.google.common.collect.Streams; import io.airlift.json.JsonCodec; import io.airlift.slice.Slice; +import io.airlift.units.DataSize; import io.trino.filesystem.TrinoFileSystem; import io.trino.plugin.iceberg.PartitionTransforms.ColumnTransform; import io.trino.spi.Page; import io.trino.spi.PageIndexer; import io.trino.spi.PageIndexerFactory; +import io.trino.spi.PageSorter; import io.trino.spi.TrinoException; import io.trino.spi.block.Block; import io.trino.spi.connector.ConnectorPageSink; import io.trino.spi.connector.ConnectorSession; +import io.trino.spi.connector.SortOrder; import io.trino.spi.type.BigintType; import io.trino.spi.type.BooleanType; import io.trino.spi.type.DateType; @@ -36,9 +39,11 @@ import io.trino.spi.type.SmallintType; import io.trino.spi.type.TinyintType; import io.trino.spi.type.Type; +import io.trino.spi.type.TypeManager; import io.trino.spi.type.UuidType; import io.trino.spi.type.VarbinaryType; import io.trino.spi.type.VarcharType; +import org.apache.hadoop.fs.Path; import org.apache.iceberg.MetricsConfig; import org.apache.iceberg.PartitionField; import org.apache.iceberg.PartitionSpec; @@ -46,6 +51,7 @@ import org.apache.iceberg.Schema; import org.apache.iceberg.io.LocationProvider; import org.apache.iceberg.transforms.Transform; +import org.apache.iceberg.types.Types; import java.io.Closeable; import java.util.ArrayList; @@ -62,7 +68,10 @@ import static com.google.common.base.Verify.verify; import static com.google.common.collect.ImmutableList.toImmutableList; import static io.airlift.slice.Slices.wrappedBuffer; +import static io.trino.plugin.iceberg.IcebergErrorCode.ICEBERG_INVALID_METADATA; import static io.trino.plugin.iceberg.IcebergErrorCode.ICEBERG_TOO_MANY_OPEN_PARTITIONS; +import static io.trino.plugin.iceberg.IcebergSessionProperties.isSortedWritingEnabled; +import static io.trino.plugin.iceberg.IcebergUtil.getColumns; import static io.trino.plugin.iceberg.PartitionTransforms.getColumnTransform; import static io.trino.plugin.iceberg.util.Timestamps.getTimestampTz; import static io.trino.plugin.iceberg.util.Timestamps.timestampTzToMicros; @@ -98,6 +107,16 @@ public class IcebergPageSink private final PagePartitioner pagePartitioner; private final long targetMaxFileSize; private final Map storageProperties; + private final List sortOrder; + private final boolean sortedWritingEnabled; + private final DataSize sortingFileWriterBufferSize; + private final Integer sortingFileWriterMaxOpenFiles; + private final Path tempDirectory; + private final TypeManager typeManager; + private final PageSorter pageSorter; + private final List columnTypes; + private final List sortColumnIndexes; + private final List sortOrders; private final List writers = new ArrayList<>(); private final List closedWriterRollbackActions = new ArrayList<>(); @@ -119,7 +138,12 @@ public IcebergPageSink( ConnectorSession session, IcebergFileFormat fileFormat, Map storageProperties, - int maxOpenWriters) + int maxOpenWriters, + List sortOrder, + DataSize sortingFileWriterBufferSize, + int sortingFileWriterMaxOpenFiles, + TypeManager typeManager, + PageSorter pageSorter) { requireNonNull(inputColumns, "inputColumns is null"); this.outputSchema = requireNonNull(outputSchema, "outputSchema is null"); @@ -135,6 +159,36 @@ public IcebergPageSink( this.pagePartitioner = new PagePartitioner(pageIndexerFactory, toPartitionColumns(inputColumns, partitionSpec)); this.targetMaxFileSize = IcebergSessionProperties.getTargetMaxFileSize(session); this.storageProperties = requireNonNull(storageProperties, "storageProperties is null"); + this.sortOrder = requireNonNull(sortOrder, "sortOrder is null"); + this.sortedWritingEnabled = isSortedWritingEnabled(session); + this.sortingFileWriterBufferSize = requireNonNull(sortingFileWriterBufferSize, "sortingFileWriterBufferSize is null"); + this.sortingFileWriterMaxOpenFiles = sortingFileWriterMaxOpenFiles; + String tempDirectoryPath = locationProvider.newDataLocation("trino-tmp-files"); + this.tempDirectory = new Path(tempDirectoryPath); + this.typeManager = requireNonNull(typeManager, "typeManager is null"); + this.pageSorter = requireNonNull(pageSorter, "pageSorter is null"); + this.columnTypes = getColumns(outputSchema, typeManager).stream() + .map(IcebergColumnHandle::getType) + .collect(toImmutableList()); + + if (sortedWritingEnabled) { + ImmutableList.Builder sortColumnIndexes = ImmutableList.builder(); + ImmutableList.Builder sortOrders = ImmutableList.builder(); + for (TrinoSortField sortField : sortOrder) { + Types.NestedField column = outputSchema.findField(sortField.getSourceColumnId()); + if (column == null) { + throw new TrinoException(ICEBERG_INVALID_METADATA, "Unable to find sort field source column in the table schema: " + sortField); + } + sortColumnIndexes.add(outputSchema.columns().indexOf(column)); + sortOrders.add(sortField.getSortOrder()); + } + this.sortColumnIndexes = sortColumnIndexes.build(); + this.sortOrders = sortOrders.build(); + } + else { + this.sortColumnIndexes = ImmutableList.of(); + this.sortOrders = ImmutableList.of(); + } } @Override @@ -286,7 +340,32 @@ private int[] getWriterIndexes(Page page) } Optional partitionData = getPartitionData(pagePartitioner.getColumns(), page, position); - writer = createWriter(partitionData); + // prepend query id to a file name so we can determine which files were written by which query. This is needed for opportunistic cleanup of extra files + // which may be present for successfully completing query in presence of failure recovery mechanisms. + String fileName = fileFormat.toIceberg().addExtension(session.getQueryId() + "-" + randomUUID()); + String outputPath = partitionData + .map(partition -> locationProvider.newDataLocation(partitionSpec, partition, fileName)) + .orElseGet(() -> locationProvider.newDataLocation(fileName)); + + if (!sortOrder.isEmpty() && sortedWritingEnabled) { + Path tempFilePrefix = new Path(tempDirectory, "sorting-file-writer-%s-%s".formatted(session.getQueryId(), randomUUID())); + WriteContext writerContext = createWriter(outputPath, partitionData); + IcebergFileWriter sortedFileWriter = new IcebergSortingFileWriter( + fileSystem, + tempFilePrefix, + writerContext.getWriter(), + sortingFileWriterBufferSize, + sortingFileWriterMaxOpenFiles, + columnTypes, + sortColumnIndexes, + sortOrders, + pageSorter, + typeManager.getTypeOperators()); + writer = new WriteContext(sortedFileWriter, outputPath, partitionData); + } + else { + writer = createWriter(outputPath, partitionData); + } writers.set(writerIndex, writer); memoryUsage += writer.getWriter().getMemoryUsage(); @@ -328,15 +407,8 @@ private void closeWriter(int writerIndex) commitTasks.add(wrappedBuffer(jsonCodec.toJsonBytes(task))); } - private WriteContext createWriter(Optional partitionData) + private WriteContext createWriter(String outputPath, Optional partitionData) { - // prepend query id to a file name so we can determine which files were written by which query. This is needed for opportunistic cleanup of extra files - // which may be present for successfully completing query in presence of failure recovery mechanisms. - String fileName = fileFormat.toIceberg().addExtension(session.getQueryId() + "-" + randomUUID()); - String outputPath = partitionData - .map(partition -> locationProvider.newDataLocation(partitionSpec, partition, fileName)) - .orElseGet(() -> locationProvider.newDataLocation(fileName)); - IcebergFileWriter writer = fileWriterFactory.createDataFileWriter( fileSystem, outputPath, diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSinkProvider.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSinkProvider.java index 532075ddc096..9e16385d5465 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSinkProvider.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSinkProvider.java @@ -14,10 +14,13 @@ package io.trino.plugin.iceberg; import io.airlift.json.JsonCodec; +import io.airlift.units.DataSize; import io.trino.filesystem.TrinoFileSystemFactory; +import io.trino.plugin.hive.SortingFileWriterConfig; import io.trino.plugin.iceberg.procedure.IcebergOptimizeHandle; import io.trino.plugin.iceberg.procedure.IcebergTableExecuteHandle; import io.trino.spi.PageIndexerFactory; +import io.trino.spi.PageSorter; import io.trino.spi.connector.ConnectorInsertTableHandle; import io.trino.spi.connector.ConnectorMergeSink; import io.trino.spi.connector.ConnectorMergeTableHandle; @@ -28,6 +31,7 @@ import io.trino.spi.connector.ConnectorSession; import io.trino.spi.connector.ConnectorTableExecuteHandle; import io.trino.spi.connector.ConnectorTransactionHandle; +import io.trino.spi.type.TypeManager; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.PartitionSpecParser; import org.apache.iceberg.Schema; @@ -50,6 +54,10 @@ public class IcebergPageSinkProvider private final IcebergFileWriterFactory fileWriterFactory; private final PageIndexerFactory pageIndexerFactory; private final int maxOpenPartitions; + private final DataSize sortingFileWriterBufferSize; + private final int sortingFileWriterMaxOpenFiles; + private final TypeManager typeManager; + private final PageSorter pageSorter; @Inject public IcebergPageSinkProvider( @@ -57,13 +65,20 @@ public IcebergPageSinkProvider( JsonCodec jsonCodec, IcebergFileWriterFactory fileWriterFactory, PageIndexerFactory pageIndexerFactory, - IcebergConfig config) + IcebergConfig config, + SortingFileWriterConfig sortingFileWriterConfig, + TypeManager typeManager, + PageSorter pageSorter) { this.fileSystemFactory = requireNonNull(fileSystemFactory, "fileSystemFactory is null"); this.jsonCodec = requireNonNull(jsonCodec, "jsonCodec is null"); this.fileWriterFactory = requireNonNull(fileWriterFactory, "fileWriterFactory is null"); this.pageIndexerFactory = requireNonNull(pageIndexerFactory, "pageIndexerFactory is null"); this.maxOpenPartitions = config.getMaxPartitionsPerWriter(); + this.sortingFileWriterBufferSize = sortingFileWriterConfig.getWriterSortBufferSize(); + this.sortingFileWriterMaxOpenFiles = sortingFileWriterConfig.getMaxOpenSortFiles(); + this.typeManager = requireNonNull(typeManager, "typeManager is null"); + this.pageSorter = requireNonNull(pageSorter, "pageSorter is null"); } @Override @@ -96,7 +111,12 @@ private ConnectorPageSink createPageSink(ConnectorSession session, IcebergWritab session, tableHandle.getFileFormat(), tableHandle.getStorageProperties(), - maxOpenPartitions); + maxOpenPartitions, + tableHandle.getSortOrder(), + sortingFileWriterBufferSize, + sortingFileWriterMaxOpenFiles, + typeManager, + pageSorter); } @Override @@ -122,7 +142,12 @@ public ConnectorPageSink createPageSink(ConnectorTransactionHandle transactionHa session, optimizeHandle.getFileFormat(), optimizeHandle.getTableStorageProperties(), - maxOpenPartitions); + maxOpenPartitions, + optimizeHandle.getSortOrder(), + sortingFileWriterBufferSize, + sortingFileWriterMaxOpenFiles, + typeManager, + pageSorter); case DROP_EXTENDED_STATS: case EXPIRE_SNAPSHOTS: case REMOVE_ORPHAN_FILES: diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java index 239130abce36..a21adb312157 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java @@ -82,6 +82,7 @@ public final class IcebergSessionProperties public static final String EXPIRE_SNAPSHOTS_MIN_RETENTION = "expire_snapshots_min_retention"; public static final String REMOVE_ORPHAN_FILES_MIN_RETENTION = "remove_orphan_files_min_retention"; private static final String MERGE_MANIFESTS_ON_WRITE = "merge_manifests_on_write"; + private static final String SORTED_WRITING_ENABLED = "sorted_writing_enabled"; private final List> sessionProperties; @@ -283,6 +284,11 @@ public IcebergSessionProperties( "Compact manifest files when performing write operations", true, false)) + .add(booleanProperty( + SORTED_WRITING_ENABLED, + "Enable sorted writing to tables with a specified sort order", + icebergConfig.isSortedWritingEnabled(), + false)) .build(); } @@ -468,4 +474,9 @@ public static boolean isMergeManifestsOnWrite(ConnectorSession session) { return session.getProperty(MERGE_MANIFESTS_ON_WRITE, Boolean.class); } + + public static boolean isSortedWritingEnabled(ConnectorSession session) + { + return session.getProperty(SORTED_WRITING_ENABLED, Boolean.class); + } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSortingFileWriter.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSortingFileWriter.java new file mode 100644 index 000000000000..16a54393aa74 --- /dev/null +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSortingFileWriter.java @@ -0,0 +1,114 @@ +/* + * Licensed 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 io.trino.plugin.iceberg; + +import io.airlift.units.DataSize; +import io.trino.filesystem.TrinoFileSystem; +import io.trino.plugin.hive.SortingFileWriter; +import io.trino.plugin.hive.orc.OrcFileWriterFactory; +import io.trino.spi.Page; +import io.trino.spi.PageSorter; +import io.trino.spi.connector.SortOrder; +import io.trino.spi.type.Type; +import io.trino.spi.type.TypeOperators; +import org.apache.hadoop.fs.Path; +import org.apache.iceberg.Metrics; + +import java.io.Closeable; +import java.util.List; +import java.util.Optional; + +import static java.util.Objects.requireNonNull; + +public class IcebergSortingFileWriter + implements IcebergFileWriter +{ + private final IcebergFileWriter outputWriter; + private final SortingFileWriter sortingFileWriter; + + public IcebergSortingFileWriter( + TrinoFileSystem fileSystem, + Path tempFilePrefix, + IcebergFileWriter outputWriter, + DataSize maxMemory, + int maxOpenTempFiles, + List types, + List sortFields, + List sortOrders, + PageSorter pageSorter, + TypeOperators typeOperators) + { + this.outputWriter = requireNonNull(outputWriter, "outputWriter is null"); + this.sortingFileWriter = new SortingFileWriter( + fileSystem, + tempFilePrefix, + outputWriter, + maxMemory, + maxOpenTempFiles, + types, + sortFields, + sortOrders, + pageSorter, + typeOperators, + OrcFileWriterFactory::createOrcDataSink); + } + + @Override + public Metrics getMetrics() + { + return outputWriter.getMetrics(); + } + + @Override + public long getWrittenBytes() + { + return sortingFileWriter.getWrittenBytes(); + } + + @Override + public long getMemoryUsage() + { + return sortingFileWriter.getMemoryUsage(); + } + + @Override + public void appendRows(Page dataPage) + { + sortingFileWriter.appendRows(dataPage); + } + + @Override + public Closeable commit() + { + return sortingFileWriter.commit(); + } + + @Override + public void rollback() + { + sortingFileWriter.rollback(); + } + + @Override + public long getValidationCpuNanos() + { + return sortingFileWriter.getValidationCpuNanos(); + } + + @Override + public Optional getVerificationTask() + { + return sortingFileWriter.getVerificationTask(); + } +} diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java index 55a021e78d50..b2cb24ae5c0d 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java @@ -43,6 +43,7 @@ public class IcebergTableHandle private final TableType tableType; private final Optional snapshotId; private final String tableSchemaJson; + private final List sortOrder; // Empty means the partitioning spec is not known (can be the case for certain time travel queries). private final Optional partitionSpecJson; private final int formatVersion; @@ -73,6 +74,7 @@ public static IcebergTableHandle fromJsonForDeserializationOnly( @JsonProperty("tableType") TableType tableType, @JsonProperty("snapshotId") Optional snapshotId, @JsonProperty("tableSchemaJson") String tableSchemaJson, + @JsonProperty("sortOrder") List sortOrder, @JsonProperty("partitionSpecJson") Optional partitionSpecJson, @JsonProperty("formatVersion") int formatVersion, @JsonProperty("unenforcedPredicate") TupleDomain unenforcedPredicate, @@ -90,6 +92,7 @@ public static IcebergTableHandle fromJsonForDeserializationOnly( tableType, snapshotId, tableSchemaJson, + sortOrder, partitionSpecJson, formatVersion, unenforcedPredicate, @@ -110,6 +113,7 @@ public IcebergTableHandle( TableType tableType, Optional snapshotId, String tableSchemaJson, + List sortOrder, Optional partitionSpecJson, int formatVersion, TupleDomain unenforcedPredicate, @@ -128,6 +132,7 @@ public IcebergTableHandle( this.tableType = requireNonNull(tableType, "tableType is null"); this.snapshotId = requireNonNull(snapshotId, "snapshotId is null"); this.tableSchemaJson = requireNonNull(tableSchemaJson, "schemaJson is null"); + this.sortOrder = ImmutableList.copyOf(requireNonNull(sortOrder, "sortOrder is null")); this.partitionSpecJson = requireNonNull(partitionSpecJson, "partitionSpecJson is null"); this.formatVersion = formatVersion; this.unenforcedPredicate = requireNonNull(unenforcedPredicate, "unenforcedPredicate is null"); @@ -173,6 +178,12 @@ public String getTableSchemaJson() return tableSchemaJson; } + @JsonProperty + public List getSortOrder() + { + return sortOrder; + } + @JsonProperty public Optional getPartitionSpecJson() { @@ -263,6 +274,7 @@ public IcebergTableHandle withProjectedColumns(Set projecte tableType, snapshotId, tableSchemaJson, + sortOrder, partitionSpecJson, formatVersion, unenforcedPredicate, @@ -285,6 +297,7 @@ public IcebergTableHandle withRetryMode(RetryMode retryMode) tableType, snapshotId, tableSchemaJson, + sortOrder, partitionSpecJson, formatVersion, unenforcedPredicate, @@ -307,6 +320,7 @@ public IcebergTableHandle withUpdatedColumns(List updatedCo tableType, snapshotId, tableSchemaJson, + sortOrder, partitionSpecJson, formatVersion, unenforcedPredicate, @@ -329,6 +343,7 @@ public IcebergTableHandle forOptimize(boolean recordScannedFiles, DataSize maxSc tableType, snapshotId, tableSchemaJson, + sortOrder, partitionSpecJson, formatVersion, unenforcedPredicate, @@ -360,6 +375,7 @@ public boolean equals(Object o) tableType == that.tableType && Objects.equals(snapshotId, that.snapshotId) && Objects.equals(tableSchemaJson, that.tableSchemaJson) && + Objects.equals(sortOrder, that.sortOrder) && Objects.equals(partitionSpecJson, that.partitionSpecJson) && formatVersion == that.formatVersion && Objects.equals(unenforcedPredicate, that.unenforcedPredicate) && @@ -376,7 +392,7 @@ public boolean equals(Object o) @Override public int hashCode() { - return Objects.hash(schemaName, tableName, tableType, snapshotId, tableSchemaJson, partitionSpecJson, formatVersion, unenforcedPredicate, enforcedPredicate, + return Objects.hash(schemaName, tableName, tableType, snapshotId, tableSchemaJson, sortOrder, partitionSpecJson, formatVersion, unenforcedPredicate, enforcedPredicate, projectedColumns, nameMappingJson, tableLocation, storageProperties, retryMode, updatedColumns, recordScannedFiles, maxScannedFileSize); } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableProperties.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableProperties.java index 604e730ce69d..1c525cc841b2 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableProperties.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableProperties.java @@ -41,6 +41,7 @@ public class IcebergTableProperties { public static final String FILE_FORMAT_PROPERTY = "format"; public static final String PARTITIONING_PROPERTY = "partitioning"; + public static final String SORTED_BY_PROPERTY = "sorted_by"; public static final String LOCATION_PROPERTY = "location"; public static final String FORMAT_VERSION_PROPERTY = "format_version"; public static final String ORC_BLOOM_FILTER_COLUMNS = "orc_bloom_filter_columns"; @@ -69,6 +70,15 @@ public IcebergTableProperties( false, value -> (List) value, value -> value)) + .add(new PropertyMetadata<>( + SORTED_BY_PROPERTY, + "Sorted columns", + new ArrayType(VARCHAR), + List.class, + ImmutableList.of(), + false, + value -> (List) value, + value -> value)) .add(stringProperty( LOCATION_PROPERTY, "File system location URI for the table", @@ -118,6 +128,13 @@ public static List getPartitioning(Map tableProperties) return partitioning == null ? ImmutableList.of() : ImmutableList.copyOf(partitioning); } + @SuppressWarnings("unchecked") + public static List getSortOrder(Map tableProperties) + { + List sortedBy = (List) tableProperties.get(SORTED_BY_PROPERTY); + return sortedBy == null ? ImmutableList.of() : ImmutableList.copyOf(sortedBy); + } + public static Optional getTableLocation(Map tableProperties) { return Optional.ofNullable((String) tableProperties.get(LOCATION_PROPERTY)); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java index 808282c2dd41..cc2d9dad6abc 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java @@ -52,6 +52,7 @@ import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; import org.apache.iceberg.SnapshotUpdate; +import org.apache.iceberg.SortOrder; import org.apache.iceberg.StructLike; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; @@ -100,12 +101,16 @@ import static io.trino.plugin.iceberg.IcebergTableProperties.ORC_BLOOM_FILTER_COLUMNS; import static io.trino.plugin.iceberg.IcebergTableProperties.ORC_BLOOM_FILTER_FPP; import static io.trino.plugin.iceberg.IcebergTableProperties.PARTITIONING_PROPERTY; +import static io.trino.plugin.iceberg.IcebergTableProperties.SORTED_BY_PROPERTY; import static io.trino.plugin.iceberg.IcebergTableProperties.getOrcBloomFilterColumns; import static io.trino.plugin.iceberg.IcebergTableProperties.getOrcBloomFilterFpp; import static io.trino.plugin.iceberg.IcebergTableProperties.getPartitioning; +import static io.trino.plugin.iceberg.IcebergTableProperties.getSortOrder; import static io.trino.plugin.iceberg.IcebergTableProperties.getTableLocation; import static io.trino.plugin.iceberg.PartitionFields.parsePartitionFields; import static io.trino.plugin.iceberg.PartitionFields.toPartitionFields; +import static io.trino.plugin.iceberg.SortFieldUtils.parseSortFields; +import static io.trino.plugin.iceberg.SortFieldUtils.toSortFields; import static io.trino.plugin.iceberg.TrinoMetricsReporter.TRINO_METRICS_REPORTER; import static io.trino.plugin.iceberg.TypeConverter.toIcebergType; import static io.trino.plugin.iceberg.TypeConverter.toTrinoType; @@ -204,6 +209,13 @@ public static Map getIcebergTableProperties(Table icebergTable) properties.put(PARTITIONING_PROPERTY, toPartitionFields(icebergTable.spec())); } + SortOrder sortOrder = icebergTable.sortOrder(); + // TODO: Support sort column transforms (https://github.com/trinodb/trino/issues/15088) + if (sortOrder.isSorted() && sortOrder.fields().stream().allMatch(sortField -> sortField.transform().isIdentity())) { + List sortColumnNames = toSortFields(sortOrder); + properties.put(SORTED_BY_PROPERTY, sortColumnNames); + } + if (!icebergTable.location().isEmpty()) { properties.put(LOCATION_PROPERTY, icebergTable.location()); } @@ -572,6 +584,7 @@ public static Transaction newCreateTableTransaction(TrinoCatalog catalog, Connec SchemaTableName schemaTableName = tableMetadata.getTable(); Schema schema = schemaFromMetadata(tableMetadata.getColumns()); PartitionSpec partitionSpec = parsePartitionFields(schema, getPartitioning(tableMetadata.getProperties())); + SortOrder sortOrder = parseSortFields(schema, getSortOrder(tableMetadata.getProperties())); String targetPath = getTableLocation(tableMetadata.getProperties()) .orElseGet(() -> catalog.defaultTableLocation(session, schemaTableName)); @@ -593,7 +606,7 @@ public static Transaction newCreateTableTransaction(TrinoCatalog catalog, Connec propertiesBuilder.put(TABLE_COMMENT, tableMetadata.getComment().get()); } - return catalog.newCreateTableTransaction(session, schemaTableName, schema, partitionSpec, targetPath, propertiesBuilder.buildOrThrow()); + return catalog.newCreateTableTransaction(session, schemaTableName, schema, partitionSpec, sortOrder, targetPath, propertiesBuilder.buildOrThrow()); } public static long getSnapshotIdAsOfTime(Table table, long epochMillis) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergWritableTableHandle.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergWritableTableHandle.java index b3ada32bac4d..22c885b4c465 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergWritableTableHandle.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergWritableTableHandle.java @@ -35,6 +35,7 @@ public class IcebergWritableTableHandle private final String schemaAsJson; private final Map partitionsSpecsAsJson; private final int partitionSpecId; + private final List sortOrder; private final List inputColumns; private final String outputPath; private final IcebergFileFormat fileFormat; @@ -47,6 +48,7 @@ public IcebergWritableTableHandle( @JsonProperty("schemaAsJson") String schemaAsJson, @JsonProperty("partitionSpecsAsJson") Map partitionsSpecsAsJson, @JsonProperty("partitionSpecId") int partitionSpecId, + @JsonProperty("sortOrder") List sortOrder, @JsonProperty("inputColumns") List inputColumns, @JsonProperty("outputPath") String outputPath, @JsonProperty("fileFormat") IcebergFileFormat fileFormat, @@ -57,6 +59,7 @@ public IcebergWritableTableHandle( this.schemaAsJson = requireNonNull(schemaAsJson, "schemaAsJson is null"); this.partitionsSpecsAsJson = ImmutableMap.copyOf(requireNonNull(partitionsSpecsAsJson, "partitionsSpecsAsJson is null")); this.partitionSpecId = partitionSpecId; + this.sortOrder = ImmutableList.copyOf(requireNonNull(sortOrder, "sortOrder is null")); this.inputColumns = ImmutableList.copyOf(requireNonNull(inputColumns, "inputColumns is null")); this.outputPath = requireNonNull(outputPath, "outputPath is null"); this.fileFormat = requireNonNull(fileFormat, "fileFormat is null"); @@ -89,6 +92,12 @@ public int getPartitionSpecId() return partitionSpecId; } + @JsonProperty + public List getSortOrder() + { + return sortOrder; + } + @JsonProperty public List getInputColumns() { diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/InternalIcebergConnectorFactory.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/InternalIcebergConnectorFactory.java index 44947a090c49..8de2fee63e44 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/InternalIcebergConnectorFactory.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/InternalIcebergConnectorFactory.java @@ -40,6 +40,7 @@ import io.trino.plugin.iceberg.catalog.IcebergCatalogModule; import io.trino.spi.NodeManager; import io.trino.spi.PageIndexerFactory; +import io.trino.spi.PageSorter; import io.trino.spi.classloader.ThreadContextClassLoader; import io.trino.spi.connector.Connector; import io.trino.spi.connector.ConnectorAccessControl; @@ -97,6 +98,7 @@ public static Connector createConnector( binder.bind(TypeManager.class).toInstance(context.getTypeManager()); binder.bind(PageIndexerFactory.class).toInstance(context.getPageIndexerFactory()); binder.bind(CatalogName.class).toInstance(new CatalogName(catalogName)); + binder.bind(PageSorter.class).toInstance(context.getPageSorter()); fileSystemFactory.ifPresentOrElse( factory -> binder.bind(TrinoFileSystemFactory.class).toInstance(factory), () -> binder.install(new HdfsFileSystemModule())); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/PartitionFields.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/PartitionFields.java index 7d4050c3f8b4..8c7a2bf64fac 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/PartitionFields.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/PartitionFields.java @@ -33,7 +33,7 @@ public final class PartitionFields { - private static final String UNQUOTED_IDENTIFIER = "[a-z_][a-z0-9_]*"; + private static final String UNQUOTED_IDENTIFIER = "[a-zA-Z_][a-zA-Z0-9_]*"; private static final String QUOTED_IDENTIFIER = "\"(?:\"\"|[^\"])*\""; public static final String IDENTIFIER = "(" + UNQUOTED_IDENTIFIER + "|" + QUOTED_IDENTIFIER + ")"; private static final Pattern UNQUOTED_IDENTIFIER_PATTERN = Pattern.compile(UNQUOTED_IDENTIFIER); @@ -88,7 +88,7 @@ public static void parsePartitionField(PartitionSpec.Builder builder, String fie } } - private static String fromIdentifierToColumn(String identifier) + public static String fromIdentifierToColumn(String identifier) { if (QUOTED_IDENTIFIER_PATTERN.matcher(identifier).matches()) { // We only support lowercase quoted identifiers for now. @@ -157,7 +157,7 @@ private static String fromColumnToIdentifier(String column) return quotedName(column); } - private static String quotedName(String name) + public static String quotedName(String name) { if (UNQUOTED_IDENTIFIER_PATTERN.matcher(name).matches()) { return name; diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/SortFieldUtils.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/SortFieldUtils.java new file mode 100644 index 000000000000..9e24ddd9c93e --- /dev/null +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/SortFieldUtils.java @@ -0,0 +1,124 @@ +/* + * Licensed 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 io.trino.plugin.iceberg; + +import io.trino.spi.TrinoException; +import org.apache.iceberg.NullOrder; +import org.apache.iceberg.Schema; +import org.apache.iceberg.SortField; +import org.apache.iceberg.SortOrder; +import org.apache.iceberg.SortOrderBuilder; +import org.apache.iceberg.types.Types; + +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import static com.google.common.base.MoreObjects.firstNonNull; +import static com.google.common.base.Verify.verify; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static io.trino.plugin.iceberg.IcebergTableProperties.SORTED_BY_PROPERTY; +import static io.trino.plugin.iceberg.PartitionFields.fromIdentifierToColumn; +import static io.trino.plugin.iceberg.PartitionFields.quotedName; +import static io.trino.spi.StandardErrorCode.COLUMN_NOT_FOUND; +import static io.trino.spi.StandardErrorCode.INVALID_TABLE_PROPERTY; +import static java.lang.String.format; +import static java.util.Locale.ENGLISH; + +public final class SortFieldUtils +{ + private SortFieldUtils() {} + + private static final Pattern PATTERN = Pattern.compile( + "\\s*(?" + PartitionFields.IDENTIFIER + ")" + + "(?i:\\s+(?ASC|DESC))?" + + "(?i:\\s+NULLS\\s+(?FIRST|LAST))?" + + "\\s*"); + + public static SortOrder parseSortFields(Schema schema, List fields) + { + SortOrder.Builder builder = SortOrder.builderFor(schema); + parseSortFields(builder, fields); + SortOrder sortOrder; + try { + sortOrder = builder.build(); + } + catch (RuntimeException e) { + throw new TrinoException(INVALID_TABLE_PROPERTY, "Invalid " + SORTED_BY_PROPERTY + " definition", e); + } + + Set baseColumnFieldIds = schema.columns().stream() + .map(Types.NestedField::fieldId) + .collect(toImmutableSet()); + for (SortField field : sortOrder.fields()) { + if (!baseColumnFieldIds.contains(field.sourceId())) { + throw new TrinoException(COLUMN_NOT_FOUND, "Column not found: " + schema.findColumnName(field.sourceId())); + } + } + + return sortOrder; + } + + public static void parseSortFields(SortOrderBuilder sortOrderBuilder, List fields) + { + fields.forEach(field -> parseSortField(sortOrderBuilder, field)); + } + + private static void parseSortField(SortOrderBuilder builder, String field) + { + Matcher matcher = PATTERN.matcher(field); + if (!matcher.matches()) { + throw new IllegalArgumentException("Unable to parse sort field: [%s]".formatted(field)); + } + + String columnName = fromIdentifierToColumn(matcher.group("identifier")); + + boolean ascending = switch (firstNonNull(matcher.group("ordering"), "ASC").toUpperCase(ENGLISH)) { + case "ASC" -> true; + case "DESC" -> false; + default -> throw new IllegalStateException("Unexpected ordering value"); // Unreachable + }; + + String nullOrderDefault = ascending ? "FIRST" : "LAST"; + NullOrder nullOrder = switch (firstNonNull(matcher.group("nullOrder"), nullOrderDefault).toUpperCase(ENGLISH)) { + case "FIRST" -> NullOrder.NULLS_FIRST; + case "LAST" -> NullOrder.NULLS_LAST; + default -> throw new IllegalStateException("Unexpected null ordering value"); // Unreachable + }; + + if (ascending) { + builder.asc(columnName, nullOrder); + } + else { + builder.desc(columnName, nullOrder); + } + } + + public static List toSortFields(SortOrder spec) + { + return spec.fields().stream() + .map(field -> toSortField(spec, field)) + .collect(toImmutableList()); + } + + private static String toSortField(SortOrder spec, SortField field) + { + verify(field.transform().isIdentity(), "Iceberg sort transforms are not supported"); + + String name = quotedName(spec.schema().findColumnName(field.sourceId())); + return format("%s %s %s", name, field.direction(), field.nullOrder()); + } +} diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoSortField.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoSortField.java new file mode 100644 index 000000000000..117c9df31386 --- /dev/null +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoSortField.java @@ -0,0 +1,93 @@ +/* + * Licensed 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 io.trino.plugin.iceberg; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import io.trino.spi.connector.SortOrder; +import org.apache.iceberg.SortField; + +import java.util.Objects; + +import static com.google.common.base.MoreObjects.toStringHelper; +import static java.util.Objects.requireNonNull; + +public class TrinoSortField +{ + private final int sourceColumnId; + private final SortOrder sortOrder; + + @JsonCreator + public TrinoSortField(@JsonProperty("sourceColumnId") int sourceColumnId, @JsonProperty("sortOrder") SortOrder sortOrder) + { + this.sourceColumnId = sourceColumnId; + this.sortOrder = requireNonNull(sortOrder, "sortOrder is null"); + } + + public static TrinoSortField fromIceberg(SortField sortField) + { + SortOrder sortOrder = switch (sortField.direction()) { + case ASC -> switch (sortField.nullOrder()) { + case NULLS_FIRST -> SortOrder.ASC_NULLS_FIRST; + case NULLS_LAST -> SortOrder.ASC_NULLS_LAST; + }; + case DESC -> switch (sortField.nullOrder()) { + case NULLS_FIRST -> SortOrder.DESC_NULLS_FIRST; + case NULLS_LAST -> SortOrder.DESC_NULLS_LAST; + }; + }; + + return new TrinoSortField(sortField.sourceId(), sortOrder); + } + + @JsonProperty + public int getSourceColumnId() + { + return sourceColumnId; + } + + @JsonProperty + public SortOrder getSortOrder() + { + return sortOrder; + } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + TrinoSortField that = (TrinoSortField) o; + return sourceColumnId == that.sourceColumnId && sortOrder == that.sortOrder; + } + + @Override + public int hashCode() + { + return Objects.hash(sourceColumnId, sortOrder); + } + + @Override + public String toString() + { + return toStringHelper(this) + .add("sourceColumnId", sourceColumnId) + .add("sortOrder", sortOrder) + .toString(); + } +} diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java index 7dbb20a4cfe5..88ec1508be7d 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java @@ -34,6 +34,7 @@ import org.apache.iceberg.AppendFiles; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; +import org.apache.iceberg.SortOrder; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableOperations; @@ -156,11 +157,12 @@ protected Transaction newCreateTableTransaction( SchemaTableName schemaTableName, Schema schema, PartitionSpec partitionSpec, + SortOrder sortOrder, String location, Map properties, Optional owner) { - TableMetadata metadata = newTableMetadata(schema, partitionSpec, location, properties); + TableMetadata metadata = newTableMetadata(schema, partitionSpec, sortOrder, location, properties); TableOperations ops = tableOperationsProvider.createTableOperations( this, session, diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java index 31a44eb11c03..0e9333802b42 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java @@ -23,6 +23,7 @@ import io.trino.spi.security.TrinoPrincipal; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; +import org.apache.iceberg.SortOrder; import org.apache.iceberg.Table; import org.apache.iceberg.Transaction; @@ -70,6 +71,7 @@ Transaction newCreateTableTransaction( SchemaTableName schemaTableName, Schema schema, PartitionSpec partitionSpec, + SortOrder sortOrder, String location, Map properties); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java index d7b9904c8d18..ac2b52663b4c 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java @@ -63,6 +63,7 @@ import org.apache.iceberg.BaseTable; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; +import org.apache.iceberg.SortOrder; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableMetadataParser; @@ -377,6 +378,7 @@ public Transaction newCreateTableTransaction( SchemaTableName schemaTableName, Schema schema, PartitionSpec partitionSpec, + SortOrder sortOrder, String location, Map properties) { @@ -385,6 +387,7 @@ public Transaction newCreateTableTransaction( schemaTableName, schema, partitionSpec, + sortOrder, location, properties, Optional.of(session.getUser())); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java index d1faf40cba0f..c4f56efe1c0f 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java @@ -46,6 +46,7 @@ import org.apache.iceberg.BaseTable; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; +import org.apache.iceberg.SortOrder; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.Transaction; @@ -254,6 +255,7 @@ public Transaction newCreateTableTransaction( SchemaTableName schemaTableName, Schema schema, PartitionSpec partitionSpec, + SortOrder sortOrder, String location, Map properties) { @@ -262,6 +264,7 @@ public Transaction newCreateTableTransaction( schemaTableName, schema, partitionSpec, + sortOrder, location, properties, isUsingSystemSecurity ? Optional.empty() : Optional.of(session.getUser())); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java index 2224b7432fae..648de291c61d 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java @@ -34,6 +34,7 @@ import org.apache.iceberg.BaseTable; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; +import org.apache.iceberg.SortOrder; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.Transaction; @@ -172,7 +173,14 @@ private List listNamespaces(ConnectorSession session, Optional n } @Override - public Transaction newCreateTableTransaction(ConnectorSession session, SchemaTableName schemaTableName, Schema schema, PartitionSpec partitionSpec, String location, Map properties) + public Transaction newCreateTableTransaction( + ConnectorSession session, + SchemaTableName schemaTableName, + Schema schema, + PartitionSpec partitionSpec, + SortOrder sortOrder, + String location, + Map properties) { if (!listNamespaces(session, Optional.of(schemaTableName.getSchemaName())).contains(schemaTableName.getSchemaName())) { throw new SchemaNotFoundException(schemaTableName.getSchemaName()); @@ -182,6 +190,7 @@ public Transaction newCreateTableTransaction(ConnectorSession session, SchemaTab schemaTableName, schema, partitionSpec, + sortOrder, location, properties, Optional.of(session.getUser())); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java index b98d26b690ec..3e56fe326d03 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java @@ -35,6 +35,7 @@ import org.apache.iceberg.BaseTable; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; +import org.apache.iceberg.SortOrder; import org.apache.iceberg.Table; import org.apache.iceberg.Transaction; import org.apache.iceberg.catalog.Namespace; @@ -195,11 +196,13 @@ public Transaction newCreateTableTransaction( SchemaTableName schemaTableName, Schema schema, PartitionSpec partitionSpec, + SortOrder sortOrder, String location, Map properties) { return restSessionCatalog.buildTable(convert(session), toIdentifier(schemaTableName), schema) .withPartitionSpec(partitionSpec) + .withSortOrder(sortOrder) .withLocation(location) .withProperties(properties) .createTransaction(); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/procedure/IcebergOptimizeHandle.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/procedure/IcebergOptimizeHandle.java index 62d81e06f2e9..b64238376536 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/procedure/IcebergOptimizeHandle.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/procedure/IcebergOptimizeHandle.java @@ -20,6 +20,7 @@ import io.airlift.units.DataSize; import io.trino.plugin.iceberg.IcebergColumnHandle; import io.trino.plugin.iceberg.IcebergFileFormat; +import io.trino.plugin.iceberg.TrinoSortField; import java.util.List; import java.util.Map; @@ -35,6 +36,7 @@ public class IcebergOptimizeHandle private final String schemaAsJson; private final String partitionSpecAsJson; private final List tableColumns; + private final List sortOrder; private final IcebergFileFormat fileFormat; private final Map tableStorageProperties; private final DataSize maxScannedFileSize; @@ -46,6 +48,7 @@ public IcebergOptimizeHandle( String schemaAsJson, String partitionSpecAsJson, List tableColumns, + List sortOrder, IcebergFileFormat fileFormat, Map tableStorageProperties, DataSize maxScannedFileSize, @@ -55,6 +58,7 @@ public IcebergOptimizeHandle( this.schemaAsJson = requireNonNull(schemaAsJson, "schemaAsJson is null"); this.partitionSpecAsJson = requireNonNull(partitionSpecAsJson, "partitionSpecAsJson is null"); this.tableColumns = ImmutableList.copyOf(requireNonNull(tableColumns, "tableColumns is null")); + this.sortOrder = ImmutableList.copyOf(requireNonNull(sortOrder, "sortOrder is null")); this.fileFormat = requireNonNull(fileFormat, "fileFormat is null"); this.tableStorageProperties = ImmutableMap.copyOf(requireNonNull(tableStorageProperties, "tableStorageProperties is null")); this.maxScannedFileSize = requireNonNull(maxScannedFileSize, "maxScannedFileSize is null"); @@ -85,6 +89,12 @@ public List getTableColumns() return tableColumns; } + @JsonProperty + public List getSortOrder() + { + return sortOrder; + } + @JsonProperty public IcebergFileFormat getFileFormat() { diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java index 0667c161d743..399f2aac0acf 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java @@ -14,6 +14,7 @@ package io.trino.plugin.iceberg; import com.google.common.collect.ImmutableList; +import io.trino.Session; import io.trino.testing.BaseConnectorSmokeTest; import io.trino.testing.TestingConnectorBehavior; import io.trino.testing.sql.TestTable; @@ -28,6 +29,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static io.airlift.concurrent.MoreFutures.getFutureValue; +import static io.trino.plugin.iceberg.IcebergTestUtils.withSmallRowGroups; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.DROP_TABLE; import static io.trino.testing.TestingAccessControlManager.privilege; import static io.trino.testing.TestingNames.randomNameSuffix; @@ -414,6 +416,44 @@ public void testCreateTableWithNonExistingSchemaVerifyLocation() .as("location should not exist").isFalse(); } + @Test + public void testSortedNationTable() + { + Session withSmallRowGroups = withSmallRowGroups(getSession()); + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_sorted_nation_table", + "WITH (sorted_by = ARRAY['comment'], format = '" + format.name() + "') AS SELECT * FROM nation WITH NO DATA")) { + assertUpdate(withSmallRowGroups, "INSERT INTO " + table.getName() + " SELECT * FROM nation", 25); + for (Object filePath : computeActual("SELECT file_path from \"" + table.getName() + "$files\"").getOnlyColumnAsSet()) { + assertTrue(isFileSorted((String) filePath, "comment")); + } + assertQuery("SELECT * FROM " + table.getName(), "SELECT * FROM nation"); + } + } + + @Test + public void testFileSortingWithLargerTable() + { + // Using a larger table forces buffered data to be written to disk + Session withSmallRowGroups = withSmallRowGroups(getSession()); + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_sorted_lineitem_table", + "WITH (sorted_by = ARRAY['comment'], format = '" + format.name() + "') AS SELECT * FROM lineitem WITH NO DATA")) { + assertUpdate( + withSmallRowGroups, + "INSERT INTO " + table.getName() + " SELECT * FROM lineitem", + "VALUES 60175"); + for (Object filePath : computeActual("SELECT file_path from \"" + table.getName() + "$files\"").getOnlyColumnAsSet()) { + assertTrue(isFileSorted((String) filePath, "comment")); + } + assertQuery("SELECT * FROM " + table.getName(), "SELECT * FROM lineitem"); + } + } + + protected abstract boolean isFileSorted(String path, String sortColumnName); + private String getTableLocation(String tableName) { return (String) computeScalar("SELECT DISTINCT regexp_replace(\"$path\", '/[^/]*/[^/]*$', '') FROM " + tableName); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java index 543edc3d9a1a..6ef47de1462a 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java @@ -66,6 +66,7 @@ import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; @@ -99,6 +100,7 @@ import static io.trino.plugin.iceberg.IcebergQueryRunner.ICEBERG_CATALOG; import static io.trino.plugin.iceberg.IcebergSessionProperties.EXTENDED_STATISTICS_ENABLED; import static io.trino.plugin.iceberg.IcebergSplitManager.ICEBERG_DOMAIN_COMPACTION_THRESHOLD; +import static io.trino.plugin.iceberg.IcebergTestUtils.withSmallRowGroups; import static io.trino.plugin.iceberg.IcebergUtil.TRINO_QUERY_ID_NAME; import static io.trino.spi.predicate.Domain.multipleValues; import static io.trino.spi.predicate.Domain.singleValue; @@ -162,6 +164,8 @@ protected IcebergQueryRunner.Builder createQueryRunnerBuilder() return IcebergQueryRunner.builder() .setIcebergProperties(ImmutableMap.builder() .put("iceberg.file-format", format.name()) + // Allows testing the sorting writer flushing to the file system with smaller tables + .put("iceberg.writer-sort-buffer-size", "1MB") .buildOrThrow()) .setInitialTables(ImmutableList.>builder() .addAll(REQUIRED_TPCH_TABLES) @@ -1088,6 +1092,284 @@ public void testCreatePartitionedTableWithQuotedIdentifierCasing(String columnNa } } + @Test + public void testSortByAllTypes() + { + String tableName = "test_sort_by_all_types_" + randomNameSuffix(); + assertUpdate("" + + "CREATE TABLE " + tableName + " (" + + " a_boolean boolean, " + + " an_integer integer, " + + " a_bigint bigint, " + + " a_real real, " + + " a_double double, " + + " a_short_decimal decimal(5,2), " + + " a_long_decimal decimal(38,20), " + + " a_varchar varchar, " + + " a_varbinary varbinary, " + + " a_date date, " + + " a_time time(6), " + + " a_timestamp timestamp(6), " + + " a_timestamptz timestamp(6) with time zone, " + + " a_uuid uuid, " + + " a_row row(id integer , vc varchar), " + + " an_array array(varchar), " + + " a_map map(integer, varchar) " + + ") " + + "WITH (" + + "sorted_by = ARRAY[" + + " 'a_boolean', " + + " 'an_integer', " + + " 'a_bigint', " + + " 'a_real', " + + " 'a_double', " + + " 'a_short_decimal', " + + " 'a_long_decimal', " + + " 'a_varchar', " + + " 'a_varbinary', " + + " 'a_date', " + + " 'a_time', " + + " 'a_timestamp', " + + " 'a_timestamptz', " + + " 'a_uuid'" + + " ]" + + ")"); + String values = "(" + + "true, " + + "1, " + + "BIGINT '2', " + + "REAL '3.0', " + + "DOUBLE '4.0', " + + "DECIMAL '5.00', " + + "DECIMAL '6.00', " + + "'seven', " + + "X'88888888', " + + "DATE '2022-09-09', " + + "TIME '10:10:10.000000', " + + "TIMESTAMP '2022-11-11 11:11:11.000000', " + + "TIMESTAMP '2022-11-11 11:11:11.000000 UTC', " + + "UUID '12121212-1212-1212-1212-121212121212', " + + "ROW(13, 'thirteen'), " + + "ARRAY['four', 'teen'], " + + "MAP(ARRAY[15], ARRAY['fifteen']))"; + String highValues = "(" + + "true, " + + "999999999, " + + "BIGINT '999999999', " + + "REAL '999.999', " + + "DOUBLE '999.999', " + + "DECIMAL '999.99', " + + "DECIMAL '6.00', " + + "'zzzzzzzzzzzzzz', " + + "X'FFFFFFFF', " + + "DATE '2099-12-31', " + + "TIME '23:59:59.999999', " + + "TIMESTAMP '2099-12-31 23:59:59.000000', " + + "TIMESTAMP '2099-12-31 23:59:59.000000 UTC', " + + "UUID 'FFFFFFFF-FFFF-FFFF-FFFF-FFFFFFFFFFFF', " + + "ROW(999, 'zzzzzzzz'), " + + "ARRAY['zzzz', 'zzzz'], " + + "MAP(ARRAY[999], ARRAY['zzzz']))"; + String lowValues = "(" + + "false, " + + "0, " + + "BIGINT '0', " + + "REAL '0', " + + "DOUBLE '0', " + + "DECIMAL '0', " + + "DECIMAL '0', " + + "'', " + + "X'00000000', " + + "DATE '2000-01-01', " + + "TIME '00:00:00.000000', " + + "TIMESTAMP '2000-01-01 00:00:00.000000', " + + "TIMESTAMP '2000-01-01 00:00:00.000000 UTC', " + + "UUID '00000000-0000-0000-0000-000000000000', " + + "ROW(0, ''), " + + "ARRAY['', ''], " + + "MAP(ARRAY[0], ARRAY['']))"; + + assertUpdate("INSERT INTO " + tableName + " VALUES " + values + ", " + highValues + ", " + lowValues, 3); + dropTable(tableName); + } + + @Test + public void testEmptySortedByList() + { + String tableName = "test_empty_sorted_by_list_" + randomNameSuffix(); + assertUpdate("" + + "CREATE TABLE " + tableName + " (a_boolean boolean, an_integer integer) " + + " WITH (partitioning = ARRAY['an_integer'], sorted_by = ARRAY[])"); + dropTable(tableName); + } + + @Test(dataProvider = "sortedTableWithQuotedIdentifierCasing") + public void testCreateSortedTableWithQuotedIdentifierCasing(String columnName, String sortField) + { + String tableName = "test_create_sorted_table_with_quotes_" + randomNameSuffix(); + assertUpdate(format("CREATE TABLE %s (%s bigint) WITH (sorted_by = ARRAY['%s'])", tableName, columnName, sortField)); + dropTable(tableName); + } + + @DataProvider(name = "sortedTableWithQuotedIdentifierCasing") + public static Object[][] sortedTableWithQuotedIdentifierCasing() + { + return new Object[][] { + {"col", "col"}, + {"COL", "col"}, + {"\"col\"", "col"}, + {"\"COL\"", "col"}, + {"col", "\"col\""}, + {"COL", "\"col\""}, + {"\"col\"", "\"col\""}, + {"\"COL\"", "\"col\""}, + }; + } + + @Test(dataProvider = "sortedTableWithSortTransform") + public void testCreateSortedTableWithSortTransform(String columnName, String sortField) + { + String tableName = "test_sort_with_transform_" + randomNameSuffix(); + assertThatThrownBy(() -> query(format("CREATE TABLE %s (%s TIMESTAMP(6)) WITH (sorted_by = ARRAY['%s'])", tableName, columnName, sortField))) + .hasMessageContaining("Unable to parse sort field"); + } + + @DataProvider(name = "sortedTableWithSortTransform") + public static Object[][] sortedTableWithSortTransform() + { + return new Object[][] { + {"col", "bucket(col, 3)"}, + {"col", "bucket(\"col\", 3)"}, + {"col", "truncate(col, 3)"}, + {"col", "year(col)"}, + {"col", "month(col)"}, + {"col", "date(col)"}, + {"col", "hour(col)"}, + }; + } + + @Test + public void testSortOrderChange() + { + Session withSmallRowGroups = withSmallRowGroups(getSession()); + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_sort_order_change", + "WITH (sorted_by = ARRAY['comment']) AS SELECT * FROM nation WITH NO DATA")) { + assertUpdate(withSmallRowGroups, "INSERT INTO " + table.getName() + " SELECT * FROM nation", 25); + Set sortedByComment = new HashSet<>(); + computeActual("SELECT file_path from \"" + table.getName() + "$files\"").getOnlyColumnAsSet() + .forEach(fileName -> sortedByComment.add((String) fileName)); + + assertUpdate("ALTER TABLE " + table.getName() + " SET PROPERTIES sorted_by = ARRAY['name']"); + assertUpdate(withSmallRowGroups, "INSERT INTO " + table.getName() + " SELECT * FROM nation", 25); + + for (Object filePath : computeActual("SELECT file_path from \"" + table.getName() + "$files\"").getOnlyColumnAsSet()) { + String path = (String) filePath; + if (sortedByComment.contains(path)) { + assertTrue(isFileSorted(path, "comment")); + } + else { + assertTrue(isFileSorted(path, "name")); + } + } + assertQuery("SELECT * FROM " + table.getName(), "SELECT * FROM nation UNION ALL SELECT * FROM nation"); + } + } + + @Test + public void testSortingDisabled() + { + Session withSortingDisabled = Session.builder(withSmallRowGroups(getSession())) + .setCatalogSessionProperty(ICEBERG_CATALOG, "sorted_writing_enabled", "false") + .build(); + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_sorting_disabled", + "WITH (sorted_by = ARRAY['comment']) AS SELECT * FROM nation WITH NO DATA")) { + assertUpdate(withSortingDisabled, "INSERT INTO " + table.getName() + " SELECT * FROM nation", 25); + for (Object filePath : computeActual("SELECT file_path from \"" + table.getName() + "$files\"").getOnlyColumnAsSet()) { + assertFalse(isFileSorted((String) filePath, "comment")); + } + assertQuery("SELECT * FROM " + table.getName(), "SELECT * FROM nation"); + } + } + + @Test + public void testOptimizeWithSortOrder() + { + Session withSmallRowGroups = withSmallRowGroups(getSession()); + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_optimize_with_sort_order", + "WITH (sorted_by = ARRAY['comment']) AS SELECT * FROM nation WITH NO DATA")) { + assertUpdate("INSERT INTO " + table.getName() + " SELECT * FROM nation WHERE nationkey < 10", 10); + assertUpdate("INSERT INTO " + table.getName() + " SELECT * FROM nation WHERE nationkey >= 10 AND nationkey < 20", 10); + assertUpdate("INSERT INTO " + table.getName() + " SELECT * FROM nation WHERE nationkey >= 20", 5); + assertUpdate("ALTER TABLE " + table.getName() + " SET PROPERTIES sorted_by = ARRAY['comment']"); + assertUpdate(withSmallRowGroups, "ALTER TABLE " + table.getName() + " EXECUTE optimize"); + + for (Object filePath : computeActual("SELECT file_path from \"" + table.getName() + "$files\"").getOnlyColumnAsSet()) { + assertTrue(isFileSorted((String) filePath, "comment")); + } + assertQuery("SELECT * FROM " + table.getName(), "SELECT * FROM nation"); + } + } + + @Test + public void testUpdateWithSortOrder() + { + Session withSmallRowGroups = withSmallRowGroups(getSession()); + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_sorted_update", + "WITH (sorted_by = ARRAY['comment']) AS SELECT * FROM lineitem WITH NO DATA")) { + assertUpdate( + "INSERT INTO " + table.getName() + " SELECT * FROM lineitem", + "VALUES 60175"); + assertUpdate(withSmallRowGroups, "UPDATE " + table.getName() + " SET comment = substring(comment, 2)", 60175); + assertQuery( + "SELECT orderkey, partkey, suppkey, linenumber, quantity, extendedprice, discount, tax, returnflag, linestatus, shipdate, " + + "commitdate, receiptdate, shipinstruct, shipmode, comment FROM " + table.getName(), + "SELECT orderkey, partkey, suppkey, linenumber, quantity, extendedprice, discount, tax, returnflag, linestatus, shipdate, " + + "commitdate, receiptdate, shipinstruct, shipmode, substring(comment, 2) FROM lineitem"); + for (Object filePath : computeActual("SELECT file_path from \"" + table.getName() + "$files\"").getOnlyColumnAsSet()) { + assertTrue(isFileSorted((String) filePath, "comment")); + } + } + } + + protected abstract boolean isFileSorted(String path, String sortColumnName); + + @Test + public void testSortingOnNestedField() + { + String tableName = "test_sorting_on_nested_field" + randomNameSuffix(); + assertThatThrownBy(() -> query("CREATE TABLE " + tableName + " (nationkey BIGINT, row_t ROW(name VARCHAR, regionkey BIGINT, comment VARCHAR)) " + + "WITH (sorted_by = ARRAY['row_t.comment'])")) + .hasMessageContaining("Unable to parse sort field: [row_t.comment]"); + assertThatThrownBy(() -> query("CREATE TABLE " + tableName + " (nationkey BIGINT, row_t ROW(name VARCHAR, regionkey BIGINT, comment VARCHAR)) " + + "WITH (sorted_by = ARRAY['\"row_t\".\"comment\"'])")) + .hasMessageContaining("Unable to parse sort field: [\"row_t\".\"comment\"]"); + assertThatThrownBy(() -> query("CREATE TABLE " + tableName + " (nationkey BIGINT, row_t ROW(name VARCHAR, regionkey BIGINT, comment VARCHAR)) " + + "WITH (sorted_by = ARRAY['\"row_t.comment\"'])")) + .hasMessageContaining("Column not found: row_t.comment"); + } + + @Test + public void testDroppingSortColumn() + { + Session withSmallRowGroups = withSmallRowGroups(getSession()); + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_dropping_sort_column", + "WITH (sorted_by = ARRAY['comment']) AS SELECT * FROM nation WITH NO DATA")) { + assertUpdate(withSmallRowGroups, "INSERT INTO " + table.getName() + " SELECT * FROM nation", 25); + assertThatThrownBy(() -> query("ALTER TABLE " + table.getName() + " DROP COLUMN comment")) + .hasMessageContaining("Cannot find source column for sort field"); + } + } + @Test public void testTableComments() { @@ -4400,8 +4682,6 @@ public void testSplitPruningFromDataFileStatistics(DataMappingTestSetup testSetu } } - protected abstract Session withSmallRowGroups(Session session); - protected abstract boolean supportsRowGroupStatistics(String typeName); private void verifySplitCount(String query, int expectedSplitCount) diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java index 3dde19c49bc5..2f10e9f6dd50 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java @@ -13,14 +13,28 @@ */ package io.trino.plugin.iceberg; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import io.minio.messages.Event; import io.trino.Session; +import io.trino.filesystem.TrinoFileSystemFactory; +import io.trino.filesystem.hdfs.HdfsFileSystemFactory; +import io.trino.hdfs.ConfigurationInitializer; +import io.trino.hdfs.DynamicHdfsConfiguration; +import io.trino.hdfs.HdfsConfig; +import io.trino.hdfs.HdfsConfiguration; +import io.trino.hdfs.HdfsConfigurationInitializer; +import io.trino.hdfs.HdfsEnvironment; +import io.trino.hdfs.authentication.NoHdfsAuthentication; import io.trino.plugin.hive.containers.HiveMinioDataLake; import io.trino.plugin.hive.metastore.HiveMetastore; import io.trino.plugin.hive.metastore.thrift.BridgingHiveMetastore; +import io.trino.plugin.hive.s3.HiveS3Config; +import io.trino.plugin.hive.s3.TrinoS3ConfigurationInitializer; import io.trino.testing.QueryRunner; import io.trino.testing.minio.MinioClient; +import io.trino.tpch.TpchTable; import org.apache.iceberg.FileFormat; import org.intellij.lang.annotations.Language; import org.testng.annotations.Test; @@ -36,6 +50,7 @@ import static io.trino.testing.TestingNames.randomNameSuffix; import static io.trino.testing.containers.Minio.MINIO_ACCESS_KEY; import static io.trino.testing.containers.Minio.MINIO_SECRET_KEY; +import static io.trino.tpch.TpchTable.LINE_ITEM; import static java.lang.String.format; import static java.util.Locale.ENGLISH; import static org.assertj.core.api.Assertions.assertThat; @@ -43,6 +58,8 @@ public abstract class BaseIcebergMinioConnectorSmokeTest extends BaseIcebergConnectorSmokeTest { + protected final TrinoFileSystemFactory fileSystemFactory; + private final String schemaName; private final String bucketName; @@ -53,6 +70,13 @@ public BaseIcebergMinioConnectorSmokeTest(FileFormat format) super(format); this.schemaName = "tpch_" + format.name().toLowerCase(ENGLISH); this.bucketName = "test-iceberg-minio-smoke-test-" + randomNameSuffix(); + + ConfigurationInitializer s3Config = new TrinoS3ConfigurationInitializer(new HiveS3Config() + .setS3AwsAccessKey(MINIO_ACCESS_KEY) + .setS3AwsSecretKey(MINIO_SECRET_KEY)); + HdfsConfigurationInitializer initializer = new HdfsConfigurationInitializer(new HdfsConfig(), ImmutableSet.of(s3Config)); + HdfsConfiguration hdfsConfiguration = new DynamicHdfsConfiguration(initializer, ImmutableSet.of()); + this.fileSystemFactory = new HdfsFileSystemFactory(new HdfsEnvironment(hdfsConfiguration, new HdfsConfig(), new NoHdfsAuthentication())); } @Override @@ -75,11 +99,15 @@ protected QueryRunner createQueryRunner() .put("hive.s3.path-style-access", "true") .put("hive.s3.streaming.part-size", "5MB") .put("iceberg.register-table-procedure.enabled", "true") + .put("iceberg.writer-sort-buffer-size", "1MB") .buildOrThrow()) .setSchemaInitializer( SchemaInitializer.builder() .withSchemaName(schemaName) - .withClonedTpchTables(REQUIRED_TPCH_TABLES) + .withClonedTpchTables(ImmutableList.>builder() + .addAll(REQUIRED_TPCH_TABLES) + .add(LINE_ITEM) + .build()) .withSchemaProperties(Map.of("location", "'s3://" + bucketName + "/" + schemaName + "'")) .build()) .build(); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/IcebergTestUtils.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/IcebergTestUtils.java new file mode 100644 index 000000000000..c23260ccc02d --- /dev/null +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/IcebergTestUtils.java @@ -0,0 +1,163 @@ +/* + * Licensed 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 io.trino.plugin.iceberg; + +import io.airlift.slice.Slice; +import io.trino.Session; +import io.trino.filesystem.TrinoFileSystem; +import io.trino.filesystem.TrinoFileSystemFactory; +import io.trino.filesystem.TrinoInputFile; +import io.trino.filesystem.local.LocalInputFile; +import io.trino.orc.FileOrcDataSource; +import io.trino.orc.OrcDataSource; +import io.trino.orc.OrcReader; +import io.trino.orc.OrcReaderOptions; +import io.trino.orc.metadata.OrcColumnId; +import io.trino.orc.metadata.statistics.StringStatistics; +import io.trino.orc.metadata.statistics.StripeStatistics; +import io.trino.parquet.ParquetReaderOptions; +import io.trino.parquet.reader.MetadataReader; +import io.trino.plugin.hive.FileFormatDataSourceStats; +import io.trino.plugin.hive.parquet.TrinoParquetDataSource; +import org.apache.parquet.hadoop.metadata.BlockMetaData; +import org.apache.parquet.hadoop.metadata.ColumnChunkMetaData; +import org.apache.parquet.hadoop.metadata.ParquetMetadata; + +import java.io.File; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.util.List; +import java.util.Optional; +import java.util.function.Supplier; + +import static com.google.common.base.Verify.verify; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.Iterators.getOnlyElement; +import static com.google.common.collect.MoreCollectors.onlyElement; +import static io.trino.testing.TestingConnectorSession.SESSION; + +public final class IcebergTestUtils +{ + private IcebergTestUtils() + { } + + public static Session withSmallRowGroups(Session session) + { + return Session.builder(session) + .setCatalogSessionProperty("iceberg", "orc_writer_max_stripe_rows", "10") + .setCatalogSessionProperty("iceberg", "parquet_writer_page_size", "100B") + .setCatalogSessionProperty("iceberg", "parquet_writer_block_size", "100B") + .setCatalogSessionProperty("iceberg", "parquet_writer_batch_size", "10") + .build(); + } + + public static boolean checkOrcFileSorting(String path, String sortColumnName) + { + return checkOrcFileSorting(() -> { + try { + return new FileOrcDataSource(new File(path), new OrcReaderOptions()); + } + catch (FileNotFoundException e) { + throw new UncheckedIOException(e); + } + }, sortColumnName); + } + + public static boolean checkOrcFileSorting(TrinoFileSystemFactory fileSystemFactory, String path, String sortColumnName) + { + return checkOrcFileSorting(() -> { + try { + TrinoFileSystem fileSystem = fileSystemFactory.create(SESSION); + return new TrinoOrcDataSource(fileSystem.newInputFile(path), new OrcReaderOptions(), new FileFormatDataSourceStats()); + } + catch (IOException e) { + throw new UncheckedIOException(e); + } + }, sortColumnName); + } + + private static boolean checkOrcFileSorting(Supplier dataSourceSupplier, String sortColumnName) + { + OrcReaderOptions readerOptions = new OrcReaderOptions(); + try (OrcDataSource dataSource = dataSourceSupplier.get()) { + OrcReader orcReader = OrcReader.createOrcReader(dataSource, readerOptions).orElseThrow(); + String previousMax = null; + OrcColumnId sortColumnId = orcReader.getRootColumn().getNestedColumns().stream() + .filter(column -> column.getColumnName().equals(sortColumnName)) + .collect(onlyElement()) + .getColumnId(); + List statistics = orcReader.getMetadata().getStripeStatsList().stream() + .map(Optional::orElseThrow) + .collect(toImmutableList()); + verify(statistics.size() > 1, "Test must produce at least two row groups"); + + for (StripeStatistics stripeStatistics : statistics) { + // TODO: This only works if the sort column is a String + StringStatistics columnStatistics = stripeStatistics.getColumnStatistics().get(sortColumnId).getStringStatistics(); + + Slice minValue = columnStatistics.getMin(); + Slice maxValue = columnStatistics.getMax(); + if (minValue == null || maxValue == null) { + throw new IllegalStateException("ORC files must produce min/max stripe statistics"); + } + + if (previousMax != null && previousMax.compareTo(minValue.toStringUtf8()) > 0) { + return false; + } + + previousMax = maxValue.toStringUtf8(); + } + + return true; + } + catch (IOException e) { + throw new UncheckedIOException(e); + } + } + + public static boolean checkParquetFileSorting(String path, String sortColumnName) + { + return checkParquetFileSorting(new LocalInputFile(new File(path)), sortColumnName); + } + + @SuppressWarnings({"unchecked", "rawtypes"}) + public static boolean checkParquetFileSorting(TrinoInputFile inputFile, String sortColumnName) + { + ParquetMetadata parquetMetadata; + try { + parquetMetadata = MetadataReader.readFooter( + new TrinoParquetDataSource(inputFile, new ParquetReaderOptions(), new FileFormatDataSourceStats()), + Optional.empty()); + } + catch (IOException e) { + throw new UncheckedIOException(e); + } + + Comparable previousMax = null; + verify(parquetMetadata.getBlocks().size() > 1, "Test must produce at least two row groups"); + for (BlockMetaData blockMetaData : parquetMetadata.getBlocks()) { + ColumnChunkMetaData columnMetadata = blockMetaData.getColumns().stream() + .filter(column -> getOnlyElement(column.getPath().iterator()).equalsIgnoreCase(sortColumnName)) + .collect(onlyElement()); + if (previousMax != null) { + if (previousMax.compareTo(columnMetadata.getStatistics().genericGetMin()) > 0) { + return false; + } + } + previousMax = columnMetadata.getStatistics().genericGetMax(); + } + return true; + } +} diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAbfsConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAbfsConnectorSmokeTest.java index 8b27ba3a2695..c9065269aa73 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAbfsConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAbfsConnectorSmokeTest.java @@ -13,12 +13,26 @@ */ package io.trino.plugin.iceberg; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.io.Resources; +import io.trino.filesystem.TrinoFileSystemFactory; +import io.trino.filesystem.hdfs.HdfsFileSystemFactory; +import io.trino.hdfs.ConfigurationInitializer; +import io.trino.hdfs.DynamicHdfsConfiguration; +import io.trino.hdfs.HdfsConfig; +import io.trino.hdfs.HdfsConfiguration; +import io.trino.hdfs.HdfsConfigurationInitializer; +import io.trino.hdfs.HdfsEnvironment; +import io.trino.hdfs.authentication.NoHdfsAuthentication; +import io.trino.plugin.hive.azure.HiveAzureConfig; +import io.trino.plugin.hive.azure.TrinoAzureConfigurationInitializer; import io.trino.plugin.hive.containers.HiveHadoop; import io.trino.plugin.hive.metastore.HiveMetastore; import io.trino.plugin.hive.metastore.thrift.BridgingHiveMetastore; import io.trino.testing.QueryRunner; +import io.trino.tpch.TpchTable; import org.testng.annotations.Parameters; import org.testng.annotations.Test; @@ -30,7 +44,9 @@ import java.util.Set; import static io.trino.plugin.hive.TestingThriftHiveMetastoreBuilder.testingThriftHiveMetastoreBuilder; +import static io.trino.plugin.iceberg.IcebergTestUtils.checkOrcFileSorting; import static io.trino.testing.TestingNames.randomNameSuffix; +import static io.trino.tpch.TpchTable.LINE_ITEM; import static java.lang.String.format; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Locale.ENGLISH; @@ -48,6 +64,7 @@ public class TestIcebergAbfsConnectorSmokeTest private final String bucketName; private HiveHadoop hiveHadoop; + private TrinoFileSystemFactory fileSystemFactory; @Parameters({ "hive.hadoop2.azure-abfs-container", @@ -82,6 +99,14 @@ protected QueryRunner createQueryRunner() .build()); this.hiveHadoop.start(); + HiveAzureConfig azureConfig = new HiveAzureConfig() + .setAbfsStorageAccount(account) + .setAbfsAccessKey(accessKey); + ConfigurationInitializer azureConfigurationInitializer = new TrinoAzureConfigurationInitializer(azureConfig); + HdfsConfigurationInitializer initializer = new HdfsConfigurationInitializer(new HdfsConfig(), ImmutableSet.of(azureConfigurationInitializer)); + HdfsConfiguration hdfsConfiguration = new DynamicHdfsConfiguration(initializer, ImmutableSet.of()); + this.fileSystemFactory = new HdfsFileSystemFactory(new HdfsEnvironment(hdfsConfiguration, new HdfsConfig(), new NoHdfsAuthentication())); + return IcebergQueryRunner.builder() .setIcebergProperties( ImmutableMap.builder() @@ -92,11 +117,15 @@ protected QueryRunner createQueryRunner() .put("hive.azure.abfs-storage-account", account) .put("hive.azure.abfs-access-key", accessKey) .put("iceberg.register-table-procedure.enabled", "true") + .put("iceberg.writer-sort-buffer-size", "1MB") .buildOrThrow()) .setSchemaInitializer( SchemaInitializer.builder() .withSchemaName(schemaName) - .withClonedTpchTables(REQUIRED_TPCH_TABLES) + .withClonedTpchTables(ImmutableList.>builder() + .addAll(REQUIRED_TPCH_TABLES) + .add(LINE_ITEM) + .build()) .withSchemaProperties(Map.of("location", "'" + formatAbfsUrl(container, account, bucketName) + schemaName + "'")) .build()) .build(); @@ -158,6 +187,12 @@ protected void deleteDirectory(String location) hiveHadoop.executeInContainerFailOnError("hadoop", "fs", "-rm", "-f", "-r", location); } + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + return checkOrcFileSorting(fileSystemFactory, path, sortColumnName); + } + private static String formatAbfsUrl(String container, String account, String bucketName) { return format("abfs://%s@%s.dfs.core.windows.net/%s/", container, account, bucketName); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAvroConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAvroConnectorTest.java index 7c3eff683d2c..1c9d7538b960 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAvroConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAvroConnectorTest.java @@ -13,7 +13,6 @@ */ package io.trino.plugin.iceberg; -import io.trino.Session; import org.testng.SkipException; import static io.trino.plugin.iceberg.IcebergFileFormat.AVRO; @@ -39,14 +38,14 @@ protected boolean supportsRowGroupStatistics(String typeName) } @Override - protected Session withSmallRowGroups(Session session) + public void testIncorrectIcebergFileSizes() { - return session; + throw new SkipException("Avro does not do tail reads"); } @Override - public void testIncorrectIcebergFileSizes() + protected boolean isFileSorted(String path, String sortColumnName) { - throw new SkipException("Avro does not do tail reads"); + throw new SkipException("Unimplemented"); } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java index 5f124ce47428..e5688ce70007 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java @@ -59,7 +59,8 @@ public void testDefaults() .setTargetMaxFileSize(DataSize.of(1, GIGABYTE)) .setMinimumAssignedSplitWeight(0.05) .setMaterializedViewsStorageSchema(null) - .setRegisterTableProcedureEnabled(false)); + .setRegisterTableProcedureEnabled(false) + .setSortedWritingEnabled(true)); } @Test @@ -85,6 +86,7 @@ public void testExplicitPropertyMappings() .put("iceberg.minimum-assigned-split-weight", "0.01") .put("iceberg.materialized-views.storage-schema", "mv_storage_schema") .put("iceberg.register-table-procedure.enabled", "true") + .put("iceberg.sorted-writing-enabled", "false") .buildOrThrow(); IcebergConfig expected = new IcebergConfig() @@ -106,7 +108,8 @@ public void testExplicitPropertyMappings() .setTargetMaxFileSize(DataSize.of(1, MEGABYTE)) .setMinimumAssignedSplitWeight(0.01) .setMaterializedViewsStorageSchema("mv_storage_schema") - .setRegisterTableProcedureEnabled(true); + .setRegisterTableProcedureEnabled(true) + .setSortedWritingEnabled(false); assertFullMapping(properties, expected); } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java index 5458471b01a3..05f5b546ca21 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java @@ -13,9 +13,11 @@ */ package io.trino.plugin.iceberg; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import io.trino.plugin.hive.metastore.HiveMetastore; import io.trino.testing.QueryRunner; +import io.trino.tpch.TpchTable; import org.testng.annotations.AfterClass; import java.io.File; @@ -27,6 +29,8 @@ import static com.google.common.io.MoreFiles.deleteRecursively; import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; import static io.trino.plugin.hive.metastore.file.FileHiveMetastore.createTestingFileHiveMetastore; +import static io.trino.plugin.iceberg.IcebergTestUtils.checkOrcFileSorting; +import static io.trino.tpch.TpchTable.LINE_ITEM; import static java.lang.String.format; import static org.apache.iceberg.FileFormat.ORC; import static org.assertj.core.api.Assertions.assertThat; @@ -52,9 +56,14 @@ protected QueryRunner createQueryRunner() this.metastoreDir.deleteOnExit(); this.metastore = createTestingFileHiveMetastore(metastoreDir); return IcebergQueryRunner.builder() - .setInitialTables(REQUIRED_TPCH_TABLES) + .setInitialTables(ImmutableList.>builder() + .addAll(REQUIRED_TPCH_TABLES) + .add(LINE_ITEM) + .build()) .setMetastoreDirectory(metastoreDir) - .setIcebergProperties(ImmutableMap.of("iceberg.register-table-procedure.enabled", "true")) + .setIcebergProperties(ImmutableMap.of( + "iceberg.register-table-procedure.enabled", "true", + "iceberg.writer-sort-buffer-size", "1MB")) .build(); } @@ -102,4 +111,10 @@ protected void deleteDirectory(String location) throw new UncheckedIOException(e); } } + + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + return checkOrcFileSorting(path, sortColumnName); + } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergGcsConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergGcsConnectorSmokeTest.java index aa8b9aa0fe52..e22dc4643708 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergGcsConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergGcsConnectorSmokeTest.java @@ -13,10 +13,21 @@ */ package io.trino.plugin.iceberg; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.io.Resources; import io.airlift.log.Logger; -import io.trino.hadoop.ConfigurationInstantiator; +import io.trino.filesystem.TrinoFileSystem; +import io.trino.filesystem.TrinoFileSystemFactory; +import io.trino.filesystem.hdfs.HdfsFileSystemFactory; +import io.trino.hdfs.ConfigurationInitializer; +import io.trino.hdfs.DynamicHdfsConfiguration; +import io.trino.hdfs.HdfsConfig; +import io.trino.hdfs.HdfsConfiguration; +import io.trino.hdfs.HdfsConfigurationInitializer; +import io.trino.hdfs.HdfsEnvironment; +import io.trino.hdfs.authentication.NoHdfsAuthentication; import io.trino.plugin.hive.containers.HiveHadoop; import io.trino.plugin.hive.gcs.GoogleGcsConfigurationInitializer; import io.trino.plugin.hive.gcs.HiveGcsConfig; @@ -24,8 +35,7 @@ import io.trino.plugin.hive.metastore.thrift.BridgingHiveMetastore; import io.trino.testing.QueryRunner; import io.trino.testing.TestingConnectorBehavior; -import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.fs.FileSystem; +import io.trino.tpch.TpchTable; import org.testng.annotations.AfterClass; import org.testng.annotations.Parameters; import org.testng.annotations.Test; @@ -34,8 +44,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.UncheckedIOException; -import java.net.URI; -import java.net.URISyntaxException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.attribute.FileAttribute; @@ -44,7 +52,10 @@ import static io.trino.plugin.hive.TestingThriftHiveMetastoreBuilder.testingThriftHiveMetastoreBuilder; import static io.trino.plugin.hive.containers.HiveHadoop.HIVE3_IMAGE; +import static io.trino.plugin.iceberg.IcebergTestUtils.checkOrcFileSorting; +import static io.trino.testing.TestingConnectorSession.SESSION; import static io.trino.testing.TestingNames.randomNameSuffix; +import static io.trino.tpch.TpchTable.LINE_ITEM; import static java.lang.String.format; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Objects.requireNonNull; @@ -62,7 +73,7 @@ public class TestIcebergGcsConnectorSmokeTest private final String schema; private HiveHadoop hiveHadoop; - private FileSystem fileSystem; + private TrinoFileSystemFactory fileSystemFactory; @Parameters({"testing.gcp-storage-bucket", "testing.gcp-credentials-key"}) public TestIcebergGcsConnectorSmokeTest(String gcpStorageBucket, String gcpCredentialKey) @@ -79,38 +90,30 @@ protected QueryRunner createQueryRunner() { InputStream jsonKey = new ByteArrayInputStream(Base64.getDecoder().decode(gcpCredentialKey)); Path gcpCredentialsFile; - try { - gcpCredentialsFile = Files.createTempFile("gcp-credentials", ".json", READ_ONLY_PERMISSIONS); - gcpCredentialsFile.toFile().deleteOnExit(); - Files.write(gcpCredentialsFile, jsonKey.readAllBytes()); - - String gcpSpecificCoreSiteXmlContent = Resources.toString(Resources.getResource("hdp3.1-core-site.xml.gcs-template"), UTF_8) - .replace("%GCP_CREDENTIALS_FILE_PATH%", "/etc/hadoop/conf/gcp-credentials.json"); - - Path hadoopCoreSiteXmlTempFile = Files.createTempFile("core-site", ".xml", READ_ONLY_PERMISSIONS); - hadoopCoreSiteXmlTempFile.toFile().deleteOnExit(); - Files.writeString(hadoopCoreSiteXmlTempFile, gcpSpecificCoreSiteXmlContent); - - this.hiveHadoop = closeAfterClass(HiveHadoop.builder() - .withImage(HIVE3_IMAGE) - .withFilesToMount(ImmutableMap.of( - "/etc/hadoop/conf/core-site.xml", hadoopCoreSiteXmlTempFile.normalize().toAbsolutePath().toString(), - "/etc/hadoop/conf/gcp-credentials.json", gcpCredentialsFile.toAbsolutePath().toString())) - .build()); - this.hiveHadoop.start(); - - HiveGcsConfig gcsConfig = new HiveGcsConfig().setJsonKeyFilePath(gcpCredentialsFile.toAbsolutePath().toString()); - Configuration configuration = ConfigurationInstantiator.newEmptyConfiguration(); - new GoogleGcsConfigurationInitializer(gcsConfig).initializeConfiguration(configuration); - - this.fileSystem = FileSystem.newInstance(new URI(schemaPath()), configuration); - } - catch (IOException e) { - throw new UncheckedIOException(e); - } - catch (URISyntaxException e) { - throw new RuntimeException(e); - } + gcpCredentialsFile = Files.createTempFile("gcp-credentials", ".json", READ_ONLY_PERMISSIONS); + gcpCredentialsFile.toFile().deleteOnExit(); + Files.write(gcpCredentialsFile, jsonKey.readAllBytes()); + + String gcpSpecificCoreSiteXmlContent = Resources.toString(Resources.getResource("hdp3.1-core-site.xml.gcs-template"), UTF_8) + .replace("%GCP_CREDENTIALS_FILE_PATH%", "/etc/hadoop/conf/gcp-credentials.json"); + + Path hadoopCoreSiteXmlTempFile = Files.createTempFile("core-site", ".xml", READ_ONLY_PERMISSIONS); + hadoopCoreSiteXmlTempFile.toFile().deleteOnExit(); + Files.writeString(hadoopCoreSiteXmlTempFile, gcpSpecificCoreSiteXmlContent); + + this.hiveHadoop = closeAfterClass(HiveHadoop.builder() + .withImage(HIVE3_IMAGE) + .withFilesToMount(ImmutableMap.of( + "/etc/hadoop/conf/core-site.xml", hadoopCoreSiteXmlTempFile.normalize().toAbsolutePath().toString(), + "/etc/hadoop/conf/gcp-credentials.json", gcpCredentialsFile.toAbsolutePath().toString())) + .build()); + this.hiveHadoop.start(); + + HiveGcsConfig gcsConfig = new HiveGcsConfig().setJsonKeyFilePath(gcpCredentialsFile.toAbsolutePath().toString()); + ConfigurationInitializer configurationInitializer = new GoogleGcsConfigurationInitializer(gcsConfig); + HdfsConfigurationInitializer initializer = new HdfsConfigurationInitializer(new HdfsConfig(), ImmutableSet.of(configurationInitializer)); + HdfsConfiguration hdfsConfiguration = new DynamicHdfsConfiguration(initializer, ImmutableSet.of()); + this.fileSystemFactory = new HdfsFileSystemFactory(new HdfsEnvironment(hdfsConfiguration, new HdfsConfig(), new NoHdfsAuthentication())); return IcebergQueryRunner.builder() .setIcebergProperties(ImmutableMap.builder() @@ -119,10 +122,14 @@ protected QueryRunner createQueryRunner() .put("hive.metastore.uri", "thrift://" + hiveHadoop.getHiveMetastoreEndpoint()) .put("iceberg.file-format", format.name()) .put("iceberg.register-table-procedure.enabled", "true") + .put("iceberg.writer-sort-buffer-size", "1MB") .buildOrThrow()) .setSchemaInitializer( SchemaInitializer.builder() - .withClonedTpchTables(REQUIRED_TPCH_TABLES) + .withClonedTpchTables(ImmutableList.>builder() + .addAll(REQUIRED_TPCH_TABLES) + .add(LINE_ITEM) + .build()) .withSchemaName(schema) .withSchemaProperties(ImmutableMap.of("location", "'" + schemaPath() + "'")) .build()) @@ -132,15 +139,13 @@ protected QueryRunner createQueryRunner() @AfterClass(alwaysRun = true) public void removeTestData() { - if (fileSystem != null) { - try { - fileSystem.delete(new org.apache.hadoop.fs.Path(schemaPath()), true); - } - catch (IOException e) { - // The GCS bucket should be configured to expire objects automatically. Clean up issues do not need to fail the test. - LOG.warn(e, "Failed to clean up GCS test directory: %s", schemaPath()); - } - fileSystem = null; + try { + TrinoFileSystem fileSystem = fileSystemFactory.create(SESSION); + fileSystem.deleteDirectory(schemaPath()); + } + catch (IOException e) { + // The GCS bucket should be configured to expire objects automatically. Clean up issues do not need to fail the test. + LOG.warn(e, "Failed to clean up GCS test directory: %s", schemaPath()); } } @@ -173,7 +178,8 @@ protected String schemaPath() protected boolean locationExists(String location) { try { - return fileSystem.exists(new org.apache.hadoop.fs.Path(location)); + TrinoFileSystem fileSystem = fileSystemFactory.create(SESSION); + return fileSystem.newInputFile(location).exists(); } catch (IOException e) { throw new UncheckedIOException(e); @@ -217,10 +223,17 @@ protected String getMetadataLocation(String tableName) protected void deleteDirectory(String location) { try { - fileSystem.delete(new org.apache.hadoop.fs.Path(location), true); + TrinoFileSystem fileSystem = fileSystemFactory.create(SESSION); + fileSystem.deleteDirectory(location); } catch (IOException e) { throw new UncheckedIOException(e); } } + + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + return checkOrcFileSorting(fileSystemFactory, path, sortColumnName); + } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioAvroConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioAvroConnectorSmokeTest.java index 0b8ae4ede012..1c6ec8dd3fa1 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioAvroConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioAvroConnectorSmokeTest.java @@ -13,6 +13,8 @@ */ package io.trino.plugin.iceberg; +import org.testng.SkipException; + import static org.apache.iceberg.FileFormat.AVRO; public class TestIcebergMinioAvroConnectorSmokeTest @@ -22,4 +24,22 @@ public TestIcebergMinioAvroConnectorSmokeTest() { super(AVRO); } + + @Override + public void testSortedNationTable() + { + throw new SkipException("Avro does not support file sorting"); + } + + @Override + public void testFileSortingWithLargerTable() + { + throw new SkipException("Avro does not support file sorting"); + } + + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + throw new IllegalStateException("File sorting tests should be skipped for Avro"); + } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioOrcConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioOrcConnectorSmokeTest.java index 7fccf3732dfd..51fb1daf3298 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioOrcConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioOrcConnectorSmokeTest.java @@ -13,6 +13,7 @@ */ package io.trino.plugin.iceberg; +import static io.trino.plugin.iceberg.IcebergTestUtils.checkOrcFileSorting; import static org.apache.iceberg.FileFormat.ORC; public class TestIcebergMinioOrcConnectorSmokeTest @@ -22,4 +23,10 @@ public TestIcebergMinioOrcConnectorSmokeTest() { super(ORC); } + + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + return checkOrcFileSorting(fileSystemFactory, path, sortColumnName); + } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioParquetConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioParquetConnectorSmokeTest.java index bdd7f2a25837..200c5aa67d16 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioParquetConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMinioParquetConnectorSmokeTest.java @@ -13,6 +13,10 @@ */ package io.trino.plugin.iceberg; +import io.trino.filesystem.TrinoFileSystem; + +import static io.trino.plugin.iceberg.IcebergTestUtils.checkParquetFileSorting; +import static io.trino.testing.TestingConnectorSession.SESSION; import static org.apache.iceberg.FileFormat.PARQUET; public class TestIcebergMinioParquetConnectorSmokeTest @@ -22,4 +26,11 @@ public TestIcebergMinioParquetConnectorSmokeTest() { super(PARQUET); } + + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + TrinoFileSystem fileSystem = fileSystemFactory.create(SESSION); + return checkParquetFileSorting(fileSystem.newInputFile(path), sortColumnName); + } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergNodeLocalDynamicSplitPruning.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergNodeLocalDynamicSplitPruning.java index 7408ec38f291..5d00d5437b8d 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergNodeLocalDynamicSplitPruning.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergNodeLocalDynamicSplitPruning.java @@ -180,6 +180,7 @@ private static ConnectorPageSource createTestingPageSource(HiveTransactionHandle TableType.DATA, Optional.empty(), SchemaParser.toJson(TABLE_SCHEMA), + ImmutableList.of(), Optional.of(PartitionSpecParser.toJson(PartitionSpec.unpartitioned())), 2, TupleDomain.withColumnDomains(ImmutableMap.of(KEY_ICEBERG_COLUMN_HANDLE, Domain.singleValue(INTEGER, (long) KEY_COLUMN_VALUE))), diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergOrcConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergOrcConnectorTest.java index 4944e2b6ae10..eca20205d4a0 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergOrcConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergOrcConnectorTest.java @@ -13,7 +13,6 @@ */ package io.trino.plugin.iceberg; -import io.trino.Session; import io.trino.testing.sql.TestTable; import org.testng.annotations.Test; @@ -23,6 +22,7 @@ import static com.google.common.io.Resources.getResource; import static io.trino.plugin.iceberg.IcebergFileFormat.ORC; +import static io.trino.plugin.iceberg.IcebergTestUtils.checkOrcFileSorting; import static java.lang.String.format; import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; import static org.assertj.core.api.Assertions.assertThat; @@ -50,11 +50,9 @@ protected boolean supportsRowGroupStatistics(String typeName) } @Override - protected Session withSmallRowGroups(Session session) + protected boolean isFileSorted(String path, String sortColumnName) { - return Session.builder(session) - .setCatalogSessionProperty("iceberg", "orc_writer_max_stripe_rows", "10") - .build(); + return checkOrcFileSorting(path, sortColumnName); } @Test diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergParquetConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergParquetConnectorTest.java index aca6c7943d10..7764143f5c7d 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergParquetConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergParquetConnectorTest.java @@ -13,7 +13,6 @@ */ package io.trino.plugin.iceberg; -import io.trino.Session; import io.trino.testing.MaterializedResult; import io.trino.testing.sql.TestTable; import org.testng.annotations.Test; @@ -23,6 +22,8 @@ import java.util.stream.IntStream; import static io.trino.plugin.iceberg.IcebergFileFormat.PARQUET; +import static io.trino.plugin.iceberg.IcebergTestUtils.checkParquetFileSorting; +import static io.trino.plugin.iceberg.IcebergTestUtils.withSmallRowGroups; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.testng.Assert.assertEquals; @@ -66,16 +67,6 @@ public void testRowGroupResetDictionary() } } - @Override - protected Session withSmallRowGroups(Session session) - { - return Session.builder(session) - .setCatalogSessionProperty("iceberg", "parquet_writer_page_size", "100B") - .setCatalogSessionProperty("iceberg", "parquet_writer_block_size", "100B") - .setCatalogSessionProperty("iceberg", "parquet_writer_batch_size", "10") - .build(); - } - @Override protected Optional filterSetColumnTypesDataProvider(SetColumnTypeSetup setup) { @@ -95,4 +86,10 @@ public void testDropAmbiguousRowFieldCaseSensitivity() .hasMessageContaining("Error opening Iceberg split") .hasStackTraceContaining("Multiple entries with same key"); } + + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + return checkParquetFileSorting(path, sortColumnName); + } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSplitSource.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSplitSource.java index 9e8c66df7dca..e99cb0c8a4a7 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSplitSource.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSplitSource.java @@ -120,6 +120,7 @@ public void testIncompleteDynamicFilterTimeout() TableType.DATA, Optional.empty(), SchemaParser.toJson(nationTable.schema()), + ImmutableList.of(), Optional.of(PartitionSpecParser.toJson(nationTable.spec())), 1, TupleDomain.all(), diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java index 256fdcedec2e..7634e2c3f942 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java @@ -36,6 +36,7 @@ import org.apache.iceberg.PartitionField; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; +import org.apache.iceberg.SortField; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableOperations; @@ -61,6 +62,7 @@ import java.util.UUID; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.io.MoreFiles.deleteRecursively; import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; import static io.trino.plugin.hive.HiveTestUtils.HDFS_ENVIRONMENT; @@ -346,7 +348,7 @@ public void testUpdatingAllTableProperties() assertTrue(table.properties().get(TableProperties.DEFAULT_FILE_FORMAT).equalsIgnoreCase("ORC")); assertTrue(table.spec().isUnpartitioned()); - assertUpdate("ALTER TABLE " + tableName + " SET PROPERTIES format_version = 2, partitioning = ARRAY['regionkey'], format = 'PARQUET'"); + assertUpdate("ALTER TABLE " + tableName + " SET PROPERTIES format_version = 2, partitioning = ARRAY['regionkey'], format = 'PARQUET', sorted_by = ARRAY['comment']"); table = loadTable(tableName); assertEquals(table.operations().current().formatVersion(), 2); assertTrue(table.properties().get(TableProperties.DEFAULT_FILE_FORMAT).equalsIgnoreCase("PARQUET")); @@ -355,6 +357,10 @@ public void testUpdatingAllTableProperties() assertThat(partitionFields).hasSize(1); assertEquals(partitionFields.get(0).name(), "regionkey"); assertTrue(partitionFields.get(0).transform().isIdentity()); + assertTrue(table.sortOrder().isSorted()); + List sortFields = table.sortOrder().fields(); + assertEquals(sortFields.size(), 1); + assertEquals(getOnlyElement(sortFields).sourceId(), table.schema().findField("comment").fieldId()); assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation"); } @@ -362,7 +368,8 @@ public void testUpdatingAllTableProperties() public void testUnsettingAllTableProperties() { String tableName = "test_unsetting_all_table_properties_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " WITH (format_version = 1, format = 'PARQUET', partitioning = ARRAY['regionkey']) AS SELECT * FROM tpch.tiny.nation", 25); + assertUpdate("CREATE TABLE " + tableName + " WITH (format_version = 1, format = 'PARQUET', partitioning = ARRAY['regionkey'], sorted_by = ARRAY['comment']) " + + "AS SELECT * FROM tpch.tiny.nation", 25); BaseTable table = loadTable(tableName); assertEquals(table.operations().current().formatVersion(), 1); assertTrue(table.properties().get(TableProperties.DEFAULT_FILE_FORMAT).equalsIgnoreCase("PARQUET")); @@ -372,11 +379,12 @@ public void testUnsettingAllTableProperties() assertEquals(partitionFields.get(0).name(), "regionkey"); assertTrue(partitionFields.get(0).transform().isIdentity()); - assertUpdate("ALTER TABLE " + tableName + " SET PROPERTIES format_version = DEFAULT, format = DEFAULT, partitioning = DEFAULT"); + assertUpdate("ALTER TABLE " + tableName + " SET PROPERTIES format_version = DEFAULT, format = DEFAULT, partitioning = DEFAULT, sorted_by = DEFAULT"); table = loadTable(tableName); assertEquals(table.operations().current().formatVersion(), 2); assertTrue(table.properties().get(TableProperties.DEFAULT_FILE_FORMAT).equalsIgnoreCase("ORC")); assertTrue(table.spec().isUnpartitioned()); + assertTrue(table.sortOrder().isUnsorted()); assertQuery("SELECT * FROM " + tableName, "SELECT * FROM nation"); } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestSortFieldUtils.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestSortFieldUtils.java new file mode 100644 index 000000000000..49e9ec58e45c --- /dev/null +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestSortFieldUtils.java @@ -0,0 +1,139 @@ +/* + * Licensed 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 io.trino.plugin.iceberg; + +import com.google.common.collect.ImmutableList; +import org.apache.iceberg.NullOrder; +import org.apache.iceberg.Schema; +import org.apache.iceberg.SortOrder; +import org.apache.iceberg.types.Types; +import org.intellij.lang.annotations.Language; +import org.testng.annotations.Test; + +import java.util.function.Consumer; + +import static io.trino.plugin.iceberg.SortFieldUtils.parseSortFields; +import static io.trino.testing.assertions.Assert.assertEquals; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +public class TestSortFieldUtils +{ + @Test + public void testParse() + { + assertParse("order_key", sortOrder(builder -> builder.asc("order_key"))); + assertParse("order_key ASC", sortOrder(builder -> builder.asc("order_key"))); + assertParse("order_key ASC NULLS FIRST", sortOrder(builder -> builder.asc("order_key"))); + assertParse("order_key ASC NULLS FIRST", sortOrder(builder -> builder.asc("order_key", NullOrder.NULLS_FIRST))); + assertParse("order_key ASC NULLS LAST", sortOrder(builder -> builder.asc("order_key", NullOrder.NULLS_LAST))); + assertParse("order_key DESC", sortOrder(builder -> builder.desc("order_key"))); + assertParse("order_key DESC NULLS FIRST", sortOrder(builder -> builder.desc("order_key", NullOrder.NULLS_FIRST))); + assertParse("order_key DESC NULLS LAST", sortOrder(builder -> builder.desc("order_key", NullOrder.NULLS_LAST))); + assertParse("order_key DESC NULLS LAST", sortOrder(builder -> builder.desc("order_key"))); + + // lowercase + assertParse("order_key asc nulls last", sortOrder(builder -> builder.asc("order_key", NullOrder.NULLS_LAST))); + assertParse("order_key desc nulls first", sortOrder(builder -> builder.desc("order_key", NullOrder.NULLS_FIRST))); + assertParse("\"order_key\" asc nulls last", sortOrder(builder -> builder.asc("order_key", NullOrder.NULLS_LAST))); + assertParse("\"order_key\" desc nulls first", sortOrder(builder -> builder.desc("order_key", NullOrder.NULLS_FIRST))); + + // uppercase + assertParse("ORDER_KEY ASC NULLS LAST", sortOrder(builder -> builder.asc("order_key", NullOrder.NULLS_LAST))); + assertParse("ORDER_KEY DESC NULLS FIRST", sortOrder(builder -> builder.desc("order_key", NullOrder.NULLS_FIRST))); + assertDoesNotParse("\"ORDER_KEY\" ASC NULLS LAST", "Uppercase characters in identifier '\"ORDER_KEY\"' are not supported."); + assertDoesNotParse("\"ORDER_KEY\" DESC NULLS FIRST", "Uppercase characters in identifier '\"ORDER_KEY\"' are not supported."); + + // mixed case + assertParse("OrDER_keY Asc NullS LAst", sortOrder(builder -> builder.asc("order_key", NullOrder.NULLS_LAST))); + assertParse("OrDER_keY Desc NullS FIrsT", sortOrder(builder -> builder.desc("order_key", NullOrder.NULLS_FIRST))); + assertDoesNotParse("\"OrDER_keY\" Asc NullS LAst", "Uppercase characters in identifier '\"OrDER_keY\"' are not supported."); + assertDoesNotParse("\"OrDER_keY\" Desc NullS FIrsT", "Uppercase characters in identifier '\"OrDER_keY\"' are not supported."); + + assertParse("comment", sortOrder(builder -> builder.asc("comment"))); + assertParse("\"comment\"", sortOrder(builder -> builder.asc("comment"))); + assertParse("\"quoted field\"", sortOrder(builder -> builder.asc("quoted field"))); + assertParse("\"\"\"another\"\" \"\"quoted\"\" \"\"field\"\"\"", sortOrder(builder -> builder.asc("\"another\" \"quoted\" \"field\""))); + assertParse("\"\"\"another\"\" \"\"quoted\"\" \"\"field\"\"\" ASC NULLS FIRST ", sortOrder(builder -> builder.asc("\"another\" \"quoted\" \"field\""))); + assertParse("\"\"\"another\"\" \"\"quoted\"\" \"\"field\"\"\" ASC NULLS LAST ", sortOrder(builder -> builder.asc("\"another\" \"quoted\" \"field\"", NullOrder.NULLS_LAST))); + assertParse("\"\"\"another\"\" \"\"quoted\"\" \"\"field\"\"\" DESC NULLS FIRST", sortOrder(builder -> builder.desc("\"another\" \"quoted\" \"field\"", NullOrder.NULLS_FIRST))); + assertParse(" comment ", sortOrder(builder -> builder.asc("comment"))); + assertParse("comment ASC", sortOrder(builder -> builder.asc("comment"))); + assertParse(" comment ASC ", sortOrder(builder -> builder.asc("comment"))); + assertParse("comment ASC NULLS FIRST", sortOrder(builder -> builder.asc("comment"))); + assertParse(" comment ASC NULLS FIRST ", sortOrder(builder -> builder.asc("comment"))); + assertParse("comment ASC NULLS FIRST", sortOrder(builder -> builder.asc("comment", NullOrder.NULLS_FIRST))); + assertParse(" comment ASC NULLS FIRST ", sortOrder(builder -> builder.asc("comment", NullOrder.NULLS_FIRST))); + assertParse("comment ASC NULLS FIRST", sortOrder(builder -> builder.asc("comment", NullOrder.NULLS_FIRST))); + assertParse(" comment ASC NULLS FIRST ", sortOrder(builder -> builder.asc("comment", NullOrder.NULLS_FIRST))); + assertParse("comment ASC NULLS LAST", sortOrder(builder -> builder.asc("comment", NullOrder.NULLS_LAST))); + assertParse(" comment ASC NULLS LAST ", sortOrder(builder -> builder.asc("comment", NullOrder.NULLS_LAST))); + assertParse("comment DESC", sortOrder(builder -> builder.desc("comment"))); + assertParse(" comment DESC ", sortOrder(builder -> builder.desc("comment"))); + assertParse("comment DESC NULLS FIRST", sortOrder(builder -> builder.desc("comment", NullOrder.NULLS_FIRST))); + assertParse(" comment DESC NULLS FIRST ", sortOrder(builder -> builder.desc("comment", NullOrder.NULLS_FIRST))); + assertParse("comment DESC NULLS LAST", sortOrder(builder -> builder.desc("comment", NullOrder.NULLS_LAST))); + assertParse(" comment DESC NULLS LAST ", sortOrder(builder -> builder.desc("comment", NullOrder.NULLS_LAST))); + assertParse("comment DESC NULLS LAST", sortOrder(builder -> builder.desc("comment"))); + assertParse(" comment DESC NULLS LAST ", sortOrder(builder -> builder.desc("comment"))); + + assertDoesNotParse("bucket(comment, 3)"); + assertDoesNotParse("truncate(comment, 3)"); + assertDoesNotParse("year(comment)"); + assertDoesNotParse("month(comment)"); + assertDoesNotParse("day(comment)"); + assertDoesNotParse("hour(comment)"); + + assertDoesNotParse("bucket(comment, 3) ASC"); + assertDoesNotParse("bucket(comment, 3) ASC NULLS LAST"); + } + + private static void assertParse(@Language("SQL") String value, SortOrder expected) + { + assertEquals(expected.fields().size(), 1); + assertEquals(parseField(value), expected); + } + + private static void assertDoesNotParse(@Language("SQL") String value) + { + assertDoesNotParse(value, "Unable to parse sort field: [%s]".formatted(value)); + } + + private static void assertDoesNotParse(@Language("SQL") String value, String expectedMessage) + { + assertThatThrownBy(() -> parseField(value)) + .hasMessage(expectedMessage); + } + + private static SortOrder parseField(String value) + { + return sortOrder(builder -> parseSortFields(builder, ImmutableList.of(value))); + } + + private static SortOrder sortOrder(Consumer consumer) + { + Schema schema = new Schema( + Types.NestedField.required(1, "order_key", Types.LongType.get()), + Types.NestedField.required(2, "ts", Types.TimestampType.withoutZone()), + Types.NestedField.required(3, "price", Types.DoubleType.get()), + Types.NestedField.optional(4, "comment", Types.StringType.get()), + Types.NestedField.optional(5, "notes", Types.ListType.ofRequired(6, Types.StringType.get())), + Types.NestedField.optional(7, "quoted field", Types.StringType.get()), + Types.NestedField.optional(8, "quoted ts", Types.TimestampType.withoutZone()), + Types.NestedField.optional(9, "\"another\" \"quoted\" \"field\"", Types.StringType.get())); + + SortOrder.Builder builder = SortOrder.builderFor(schema); + consumer.accept(builder); + return builder.build(); + } +} diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java index 9cbf6e6e098d..3e456e386e7c 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java @@ -28,9 +28,12 @@ import io.trino.spi.security.PrincipalType; import io.trino.spi.security.TrinoPrincipal; import io.trino.spi.type.VarcharType; +import org.apache.iceberg.NullOrder; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; +import org.apache.iceberg.SortOrder; import org.apache.iceberg.Table; +import org.apache.iceberg.expressions.Expressions; import org.apache.iceberg.types.Types; import org.testng.annotations.Test; @@ -141,6 +144,7 @@ public void testCreateTable() schemaTableName, new Schema(Types.NestedField.of(1, true, "col1", Types.LongType.get())), PartitionSpec.unpartitioned(), + SortOrder.unsorted(), tableLocation, tableProperties) .commitTransaction(); @@ -153,6 +157,7 @@ public void testCreateTable() assertEquals(icebergTable.schema().columns().get(0).name(), "col1"); assertEquals(icebergTable.schema().columns().get(0).type(), Types.LongType.get()); assertEquals(icebergTable.location(), tableLocation); + assertEquals(icebergTable.sortOrder().isUnsorted(), true); assertThat(icebergTable.properties()).containsAllEntriesOf(tableProperties); catalog.dropTable(SESSION, schemaTableName); @@ -169,6 +174,74 @@ public void testCreateTable() } } + @Test + public void testCreateWithSortTable() + throws Exception + { + TrinoCatalog catalog = createTrinoCatalog(false); + String namespace = "test_create_sort_table_" + randomNameSuffix(); + String table = "tableName"; + SchemaTableName schemaTableName = new SchemaTableName(namespace, table); + try { + catalog.createNamespace(SESSION, namespace, defaultNamespaceProperties(namespace), new TrinoPrincipal(PrincipalType.USER, SESSION.getUser())); + Schema tableSchema = new Schema(Types.NestedField.of(1, true, "col1", Types.LongType.get()), + Types.NestedField.of(2, true, "col2", Types.StringType.get()), + Types.NestedField.of(3, true, "col3", Types.TimestampType.withZone()), + Types.NestedField.of(4, true, "col4", Types.StringType.get())); + + SortOrder sortOrder = SortOrder.builderFor(tableSchema) + .asc("col1") + .desc("col2", NullOrder.NULLS_FIRST) + .desc("col3") + .desc(Expressions.year("col3"), NullOrder.NULLS_LAST) + .desc(Expressions.month("col3"), NullOrder.NULLS_FIRST) + .asc(Expressions.day("col3"), NullOrder.NULLS_FIRST) + .asc(Expressions.hour("col3"), NullOrder.NULLS_FIRST) + .desc(Expressions.bucket("col2", 10), NullOrder.NULLS_FIRST) + .desc(Expressions.truncate("col4", 5), NullOrder.NULLS_FIRST).build(); + String tableLocation = arbitraryTableLocation(catalog, SESSION, schemaTableName); + catalog.newCreateTableTransaction( + SESSION, + schemaTableName, + tableSchema, + PartitionSpec.unpartitioned(), + sortOrder, + tableLocation, + ImmutableMap.of()) + .commitTransaction(); + assertThat(catalog.listTables(SESSION, Optional.of(namespace))).contains(schemaTableName); + assertThat(catalog.listTables(SESSION, Optional.empty())).contains(schemaTableName); + + Table icebergTable = catalog.loadTable(SESSION, schemaTableName); + assertEquals(icebergTable.name(), quotedTableName(schemaTableName)); + assertEquals(icebergTable.schema().columns().size(), 4); + assertEquals(icebergTable.schema().columns().get(0).name(), "col1"); + assertEquals(icebergTable.schema().columns().get(0).type(), Types.LongType.get()); + assertEquals(icebergTable.schema().columns().get(1).name(), "col2"); + assertEquals(icebergTable.schema().columns().get(1).type(), Types.StringType.get()); + assertEquals(icebergTable.location(), tableLocation); + assertEquals(icebergTable.schema().columns().get(2).name(), "col3"); + assertEquals(icebergTable.schema().columns().get(2).type(), Types.TimestampType.withZone()); + assertEquals(icebergTable.schema().columns().get(3).name(), "col4"); + assertEquals(icebergTable.schema().columns().get(3).type(), Types.StringType.get()); + assertEquals(icebergTable.location(), tableLocation); + assertEquals(icebergTable.sortOrder(), sortOrder); + + catalog.dropTable(SESSION, schemaTableName); + } + finally { + try { + if (!catalog.listTables(SESSION, Optional.of(schemaTableName.getSchemaName())).isEmpty()) { + catalog.dropTable(SESSION, schemaTableName); + } + catalog.dropNamespace(SESSION, namespace); + } + catch (RuntimeException e) { + LOG.warn("Failed to clean up namespace: %s", namespace); + } + } + } + @Test public void testRenameTable() throws Exception @@ -187,6 +260,7 @@ public void testRenameTable() sourceSchemaTableName, new Schema(Types.NestedField.of(1, true, "col1", Types.LongType.get())), PartitionSpec.unpartitioned(), + SortOrder.unsorted(), arbitraryTableLocation(catalog, SESSION, sourceSchemaTableName), ImmutableMap.of()) .commitTransaction(); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java index e1d799b5f47e..2fb484e1fe33 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java @@ -23,13 +23,25 @@ import com.amazonaws.services.s3.model.ListObjectsV2Request; import com.amazonaws.services.s3.model.ListObjectsV2Result; import com.amazonaws.services.s3.model.S3ObjectSummary; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import io.trino.filesystem.TrinoFileSystem; +import io.trino.filesystem.TrinoFileSystemFactory; +import io.trino.filesystem.hdfs.HdfsFileSystemFactory; +import io.trino.hdfs.DynamicHdfsConfiguration; +import io.trino.hdfs.HdfsConfig; +import io.trino.hdfs.HdfsConfiguration; +import io.trino.hdfs.HdfsConfigurationInitializer; +import io.trino.hdfs.HdfsEnvironment; +import io.trino.hdfs.authentication.NoHdfsAuthentication; import io.trino.plugin.hive.aws.AwsApiCallStats; import io.trino.plugin.iceberg.BaseIcebergConnectorSmokeTest; import io.trino.plugin.iceberg.IcebergQueryRunner; import io.trino.plugin.iceberg.SchemaInitializer; import io.trino.testing.QueryRunner; import io.trino.testing.sql.TestView; +import io.trino.tpch.TpchTable; import org.apache.iceberg.FileFormat; import org.testng.annotations.AfterClass; import org.testng.annotations.Parameters; @@ -39,7 +51,10 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static io.trino.plugin.hive.metastore.glue.AwsSdkUtil.getPaginatedResults; +import static io.trino.plugin.iceberg.IcebergTestUtils.checkParquetFileSorting; +import static io.trino.testing.TestingConnectorSession.SESSION; import static io.trino.testing.TestingNames.randomNameSuffix; +import static io.trino.tpch.TpchTable.LINE_ITEM; import static java.lang.String.format; import static java.util.Objects.requireNonNull; import static org.assertj.core.api.Assertions.assertThat; @@ -56,6 +71,7 @@ public class TestIcebergGlueCatalogConnectorSmokeTest private final String bucketName; private final String schemaName; private final AWSGlueAsync glueClient; + private final TrinoFileSystemFactory fileSystemFactory; @Parameters("s3.bucket") public TestIcebergGlueCatalogConnectorSmokeTest(String bucketName) @@ -64,6 +80,10 @@ public TestIcebergGlueCatalogConnectorSmokeTest(String bucketName) this.bucketName = requireNonNull(bucketName, "bucketName is null"); this.schemaName = "test_iceberg_smoke_" + randomNameSuffix(); glueClient = AWSGlueAsyncClientBuilder.defaultClient(); + + HdfsConfigurationInitializer initializer = new HdfsConfigurationInitializer(new HdfsConfig(), ImmutableSet.of()); + HdfsConfiguration hdfsConfiguration = new DynamicHdfsConfiguration(initializer, ImmutableSet.of()); + this.fileSystemFactory = new HdfsFileSystemFactory(new HdfsEnvironment(hdfsConfiguration, new HdfsConfig(), new NoHdfsAuthentication())); } @Override @@ -75,10 +95,14 @@ protected QueryRunner createQueryRunner() ImmutableMap.of( "iceberg.catalog.type", "glue", "hive.metastore.glue.default-warehouse-dir", schemaPath(), - "iceberg.register-table-procedure.enabled", "true")) + "iceberg.register-table-procedure.enabled", "true", + "iceberg.writer-sort-buffer-size", "1MB")) .setSchemaInitializer( SchemaInitializer.builder() - .withClonedTpchTables(REQUIRED_TPCH_TABLES) + .withClonedTpchTables(ImmutableList.>builder() + .addAll(REQUIRED_TPCH_TABLES) + .add(LINE_ITEM) + .build()) .withSchemaName(schemaName) .build()) .build(); @@ -224,6 +248,13 @@ protected void deleteDirectory(String location) assertThat(s3.listObjects(bucketName, location).getObjectSummaries()).isEmpty(); } + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + TrinoFileSystem fileSystem = fileSystemFactory.create(SESSION); + return checkParquetFileSorting(fileSystem.newInputFile(path), sortColumnName); + } + @Override protected String schemaPath() { diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcCatalogConnectorSmokeTest.java index 68cb099620c4..f401ed09ff94 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcCatalogConnectorSmokeTest.java @@ -13,6 +13,7 @@ */ package io.trino.plugin.iceberg.catalog.jdbc; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import io.trino.hadoop.ConfigurationInstantiator; import io.trino.plugin.iceberg.BaseIcebergConnectorSmokeTest; @@ -20,6 +21,7 @@ import io.trino.plugin.iceberg.IcebergQueryRunner; import io.trino.testing.QueryRunner; import io.trino.testing.TestingConnectorBehavior; +import io.trino.tpch.TpchTable; import org.apache.iceberg.BaseTable; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.jdbc.JdbcCatalog; @@ -33,8 +35,10 @@ import static com.google.common.io.MoreFiles.deleteRecursively; import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; +import static io.trino.plugin.iceberg.IcebergTestUtils.checkOrcFileSorting; import static io.trino.plugin.iceberg.catalog.jdbc.TestingIcebergJdbcServer.PASSWORD; import static io.trino.plugin.iceberg.catalog.jdbc.TestingIcebergJdbcServer.USER; +import static io.trino.tpch.TpchTable.LINE_ITEM; import static java.lang.String.format; import static org.apache.iceberg.CatalogProperties.CATALOG_IMPL; import static org.apache.iceberg.CatalogProperties.URI; @@ -92,9 +96,13 @@ protected QueryRunner createQueryRunner() .put("iceberg.jdbc-catalog.connection-password", PASSWORD) .put("iceberg.jdbc-catalog.catalog-name", "tpch") .put("iceberg.register-table-procedure.enabled", "true") + .put("iceberg.writer-sort-buffer-size", "1MB") .put("iceberg.jdbc-catalog.default-warehouse-dir", warehouseLocation.getAbsolutePath()) .buildOrThrow()) - .setInitialTables(REQUIRED_TPCH_TABLES) + .setInitialTables(ImmutableList.>builder() + .addAll(REQUIRED_TPCH_TABLES) + .add(LINE_ITEM) + .build()) .build(); } @@ -166,4 +174,10 @@ protected void deleteDirectory(String location) throw new UncheckedIOException(e); } } + + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + return checkOrcFileSorting(path, sortColumnName); + } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcConnectorTest.java index ff987ee1f09d..a976c3db0ddd 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcConnectorTest.java @@ -15,12 +15,12 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import io.trino.Session; import io.trino.plugin.iceberg.BaseIcebergConnectorTest; import io.trino.plugin.iceberg.IcebergConfig; import io.trino.plugin.iceberg.IcebergQueryRunner; import io.trino.testing.QueryRunner; import io.trino.tpch.TpchTable; +import org.testng.SkipException; import org.testng.annotations.Test; import java.io.File; @@ -85,6 +85,12 @@ public void testShowCreateSchema() \\)"""); } + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + throw new SkipException("Not implemented"); + } + @Override public void testRenameSchema() { @@ -263,14 +269,6 @@ protected boolean supportsRowGroupStatistics(String typeName) return !typeName.equalsIgnoreCase("varbinary"); } - @Override - protected Session withSmallRowGroups(Session session) - { - return Session.builder(session) - .setCatalogSessionProperty("iceberg", "orc_writer_max_stripe_rows", "10") - .build(); - } - @Override protected OptionalInt maxSchemaNameLength() { diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogConnectorSmokeTest.java index a290e565147d..f7645343be26 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogConnectorSmokeTest.java @@ -13,6 +13,7 @@ */ package io.trino.plugin.iceberg.catalog.rest; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import io.airlift.http.server.testing.TestingHttpServer; import io.trino.plugin.iceberg.BaseIcebergConnectorSmokeTest; @@ -20,6 +21,7 @@ import io.trino.plugin.iceberg.IcebergQueryRunner; import io.trino.testing.QueryRunner; import io.trino.testing.TestingConnectorBehavior; +import io.trino.tpch.TpchTable; import org.apache.iceberg.catalog.Catalog; import org.apache.iceberg.rest.DelegatingRestSessionCatalog; import org.assertj.core.util.Files; @@ -30,7 +32,9 @@ import static com.google.common.io.MoreFiles.deleteRecursively; import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; +import static io.trino.plugin.iceberg.IcebergTestUtils.checkOrcFileSorting; import static io.trino.plugin.iceberg.catalog.rest.RestCatalogTestUtils.backendCatalog; +import static io.trino.tpch.TpchTable.LINE_ITEM; import static java.lang.String.format; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -81,8 +85,12 @@ protected QueryRunner createQueryRunner() .put("iceberg.catalog.type", "rest") .put("iceberg.rest-catalog.uri", testServer.getBaseUrl().toString()) .put("iceberg.register-table-procedure.enabled", "true") + .put("iceberg.writer-sort-buffer-size", "1MB") .buildOrThrow()) - .setInitialTables(REQUIRED_TPCH_TABLES) + .setInitialTables(ImmutableList.>builder() + .addAll(REQUIRED_TPCH_TABLES) + .add(LINE_ITEM) + .build()) .build(); } @@ -202,6 +210,12 @@ public void testRepeatUnregisterTable() .hasMessageContaining("unregisterTable is not supported for Iceberg REST catalogs"); } + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + return checkOrcFileSorting(path, sortColumnName); + } + @Override protected void deleteDirectory(String location) { diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalogConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalogConnectorTest.java index 217dfd8d7cec..72a9acacf616 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalogConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalogConnectorTest.java @@ -13,6 +13,7 @@ */ package io.trino.plugin.iceberg.catalog.rest; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import io.airlift.http.server.testing.TestingHttpServer; import io.trino.plugin.iceberg.IcebergQueryRunner; @@ -21,6 +22,7 @@ import io.trino.testing.MaterializedRow; import io.trino.testing.QueryRunner; import io.trino.testing.TestingConnectorBehavior; +import io.trino.tpch.TpchTable; import org.apache.iceberg.catalog.Catalog; import org.apache.iceberg.rest.DelegatingRestSessionCatalog; import org.assertj.core.util.Files; @@ -33,6 +35,7 @@ import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; import static io.trino.plugin.iceberg.catalog.rest.RestCatalogTestUtils.backendCatalog; import static io.trino.testing.MaterializedResult.DEFAULT_PRECISION; +import static io.trino.tpch.TpchTable.LINE_ITEM; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -76,7 +79,10 @@ protected QueryRunner createQueryRunner() .put("iceberg.catalog.type", "rest") .put("iceberg.rest-catalog.uri", testServer.getBaseUrl().toString()) .buildOrThrow()) - .setInitialTables(REQUIRED_TPCH_TABLES) + .setInitialTables(ImmutableList.>builder() + .addAll(REQUIRED_TPCH_TABLES) + .add(LINE_ITEM) + .build()) .build(); } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/optimizer/TestConnectorPushdownRulesWithIceberg.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/optimizer/TestConnectorPushdownRulesWithIceberg.java index 814de281f761..3747044f9666 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/optimizer/TestConnectorPushdownRulesWithIceberg.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/optimizer/TestConnectorPushdownRulesWithIceberg.java @@ -153,6 +153,7 @@ public void testProjectionPushdown() DATA, Optional.of(1L), "", + ImmutableList.of(), Optional.of(""), 1, TupleDomain.all(), @@ -235,6 +236,7 @@ public void testPredicatePushdown() DATA, Optional.of(snapshotId), "", + ImmutableList.of(), Optional.of(""), 1, TupleDomain.all(), @@ -284,6 +286,7 @@ public void testColumnPruningProjectionPushdown() DATA, Optional.empty(), "", + ImmutableList.of(), Optional.of(""), 1, TupleDomain.all(), @@ -344,6 +347,7 @@ public void testPushdownWithDuplicateExpressions() DATA, Optional.of(1L), "", + ImmutableList.of(), Optional.of(""), 1, TupleDomain.all(), diff --git a/testing/trino-faulttolerant-tests/src/test/java/io/trino/faulttolerant/iceberg/TestIcebergParquetFaultTolerantExecutionConnectorTest.java b/testing/trino-faulttolerant-tests/src/test/java/io/trino/faulttolerant/iceberg/TestIcebergParquetFaultTolerantExecutionConnectorTest.java index 2bbbd2a44b55..8e3a22d476ea 100644 --- a/testing/trino-faulttolerant-tests/src/test/java/io/trino/faulttolerant/iceberg/TestIcebergParquetFaultTolerantExecutionConnectorTest.java +++ b/testing/trino-faulttolerant-tests/src/test/java/io/trino/faulttolerant/iceberg/TestIcebergParquetFaultTolerantExecutionConnectorTest.java @@ -21,6 +21,7 @@ import org.testng.annotations.AfterClass; import static io.trino.plugin.exchange.filesystem.containers.MinioStorage.getExchangeManagerProperties; +import static io.trino.plugin.iceberg.IcebergTestUtils.checkParquetFileSorting; import static io.trino.testing.FaultTolerantExecutionConnectorTestHelper.getExtraProperties; import static io.trino.testing.TestingNames.randomNameSuffix; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -68,6 +69,12 @@ public void testStatsBasedRepartitionDataOnInsert() throw new SkipException("We always get 3 partitions with FTE"); } + @Override + protected boolean isFileSorted(String path, String sortColumnName) + { + return checkParquetFileSorting(path, sortColumnName); + } + @AfterClass(alwaysRun = true) public void destroy() throws Exception diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java index a6ebb44a66e2..7aa06fd4af19 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java @@ -1233,6 +1233,48 @@ private void assertSelectsOnSpecialCharacters(String trinoTableName, String spar } } + @Test(groups = {ICEBERG, PROFILE_SPECIFIC_TESTS}) + public void testTrinoReadsSparkSortOrder() + { + String sourceTableNameBase = "test_insert_into_sorted_table_" + randomNameSuffix(); + String trinoTableName = trinoTableName(sourceTableNameBase); + String sparkTableName = sparkTableName(sourceTableNameBase); + + onSpark().executeQuery("CREATE TABLE " + sparkTableName + " (a INT, b INT, c INT) USING ICEBERG"); + onSpark().executeQuery("ALTER TABLE " + sparkTableName + " WRITE ORDERED BY b, c DESC NULLS LAST"); + + Assertions.assertThat((String) onTrino().executeQuery("SHOW CREATE TABLE " + trinoTableName).getOnlyValue()) + .contains("sorted_by = ARRAY['b ASC NULLS FIRST','c DESC NULLS LAST']"); + + onTrino().executeQuery("INSERT INTO " + trinoTableName + " VALUES (3, 2, 1), (1, 2, 3), (NULL, NULL, NULL)"); + assertThat(onSpark().executeQuery("SELECT _pos, a, b, c FROM " + sparkTableName)) + .contains( + row(0, null, null, null), + row(1, 1, 2, 3), + row(2, 3, 2, 1)); + } + + @Test(groups = {ICEBERG, PROFILE_SPECIFIC_TESTS}) + public void testTrinoIgnoresUnsupportedSparkSortOrder() + { + String sourceTableNameBase = "test_insert_into_sorted_table_" + randomNameSuffix(); + String trinoTableName = trinoTableName(sourceTableNameBase); + String sparkTableName = sparkTableName(sourceTableNameBase); + + onSpark().executeQuery("CREATE TABLE " + sparkTableName + " (a INT, b INT, c INT) USING ICEBERG"); + onSpark().executeQuery("ALTER TABLE " + sparkTableName + " WRITE ORDERED BY truncate(b, 3), a NULLS LAST"); + + Assertions.assertThat((String) onTrino().executeQuery("SHOW CREATE TABLE " + trinoTableName).getOnlyValue()) + .doesNotContain("sorted_by"); + + onTrino().executeQuery("INSERT INTO " + trinoTableName + " VALUES (3, 2333, 1), (NULL, NULL, NULL), (1, 2222, 3)"); + assertThat(onSpark().executeQuery("SELECT _pos, a, b, c FROM " + sparkTableName)) + .contains( + row(0, 1, 2222, 3), + row(1, 3, 2333, 1), + row(2, null, null, null)); + } + /** * @see TestIcebergInsert#testIcebergConcurrentInsert() */