Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add handling for root bucket table location in sync_partition_metadata #20090

Merged
merged 1 commit into from
Jan 14, 2024
Merged

Add handling for root bucket table location in sync_partition_metadata #20090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...ino-hive/src/main/java/io/trino/plugin/hive/procedure/SyncPartitionMetadataProcedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ private static Set<Location> listDirectories(TrinoFileSystem fileSystem, Locatio
private static String listedDirectoryName(Location directory, Location location)
{
String prefix = directory.path();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the description of your PR to point out that you're using the same strategy as in

trino/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystem.java

Lines 175 to 177 in f7369f6

if (!key.isEmpty() && !key.endsWith("/")) {
key += "/";
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't noticed the same strategy was already in use on another module..!

Thanks, I will update the description!

if (!prefix.endsWith("/")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix bug in sync_partition_metadata() to handle table located in the root path of s3 bucket -> Add handling for root bucket table location in sync_partition_metadata

Commit title 80 characters. Aim for conciseness.
https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I am done!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squash the commits into one pls and use a concise commit title.
You can take inspiration from my previous message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I squashed commits with a message, same as the title of this PR, following your guide! Thanks!

if (!prefix.isEmpty() && !prefix.endsWith("/")) {
prefix += "/";
}
String path = location.path();
Expand Down
28 changes: 28 additions & 0 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHive3OnDataLake.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,34 @@ public void testInsertOverwritePartitionedAndBucketedExternalTable()
assertOverwritePartition(externalTableName);
}

@Test
public void testSyncPartitionOnBucketRoot()
{
String tableName = "test_sync_partition_on_bucket_root_" + randomNameSuffix();
String fullyQualifiedTestTableName = getFullyQualifiedTestTableName(tableName);

hiveMinioDataLake.getMinioClient().putObject(
bucketName,
"hello\u0001world\nbye\u0001world".getBytes(UTF_8),
"part_key=part_val/data.txt");

assertUpdate("CREATE TABLE " + fullyQualifiedTestTableName + "(" +
" a varchar," +
" b varchar," +
" part_key varchar)" +
"WITH (" +
" external_location='s3://" + bucketName + "/'," +
" partitioned_by=ARRAY['part_key']," +
" format='TEXTFILE'" +
")");

getQueryRunner().execute("CALL system.sync_partition_metadata(schema_name => '" + HIVE_TEST_SCHEMA + "', table_name => '" + tableName + "', mode => 'ADD')");

assertQuery("SELECT * FROM " + fullyQualifiedTestTableName, "VALUES ('hello', 'world', 'part_val'), ('bye', 'world', 'part_val')");

assertUpdate("DROP TABLE " + fullyQualifiedTestTableName);
}

@Test
public void testFlushPartitionCache()
{
Expand Down
Loading