Add handling for root bucket table location in sync_partition_metadata #20090

okayhooni · 2023-12-13T04:36:11Z

Description

Currently, sync_partition_metadata() procedure cannot handle the table located in the root bucket path, due to bug on the listDirectoryName() logic
I know, using table mapped with the root bucket path is not the best practice, but we dumped some AWS logs(like cloudfront logs) into the root path of dedicated log bucket.
the same strategy was already in use on trino/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystem.java

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

cla-bot · 2023-12-13T04:36:14Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

findinpath · 2023-12-19T16:12:20Z

.../trino-hive/src/main/java/io/trino/plugin/hive/procedure/SyncPartitionMetadataProcedure.java

@@ -191,7 +191,7 @@ private static Set<Location> listDirectories(TrinoFileSystem fileSystem, Locatio
    private static String listedDirectoryName(Location directory, Location location)
    {
        String prefix = directory.path();
-        if (!prefix.endsWith("/")) {
+        if (!prefix.endsWith("/") && !prefix.equals("")) {


Can we have some test coverage for this change?

Thank you for comment..!

I cannot find any existing test suites for built-in procedures in Hive plugin to add some test case for this hotfix.

https://github.com/trinodb/trino/tree/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive

Did you mean creating new test module for this procedure and writing all the test cases for it..?
(Is it okay to maintain other hive procedures without test suites..?)

We have so called product tests that can be used to check the accuracy of sync_partition_metadata procedure

https://github.com/trinodb/trino/blob/33dd20a8c104d358f5b6d38e8a0405f8b0ced944/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/BaseTestSyncPartitionMetadata.java

You can run the hdfs related product test in this way:

testing/bin/ptl test run --environment multinode -- -t io.trino.tests.product.hive.TestHdfsSyncPartitionMetadata

However, you can use for the purpose of testing your changes HiveMinioDataLake utility.
See for example

trino/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHive3OnDataLake.java

Lines 1764 to 1785 in 33dd20a

public void testPartitionedTableExternalLocationOnTopOfTheBucket()

{

String topBucketName = "test-hive-partitioned-top-of-the-bucket-" + randomNameSuffix();

hiveMinioDataLake.getMinio().createBucket(topBucketName);

String tableName = "test_external_location_top_of_the_bucket_" + randomNameSuffix();

assertUpdate(format(

"CREATE TABLE %s (" +

" a_varchar varchar, " +

" pkey integer) " +

"WITH (" +

" external_location='%s'," +

" partitioned_by=ARRAY['pkey'])",

tableName,

format("s3://%s/", topBucketName)));

assertUpdate("INSERT INTO " + tableName + " VALUES ('a', 1) , ('b', 1), ('c', 2), ('d', 2)", 4);

assertQuery("SELECT * FROM " + tableName, "VALUES ('a', 1), ('b',1), ('c', 2), ('d', 2)");

assertUpdate("DELETE FROM " + tableName + " where pkey = 2");

assertQuery("SELECT * FROM " + tableName, "VALUES ('a', 1), ('b',1)");

assertUpdate("DROP TABLE " + tableName);

}

Note that you can make use of io.trino.plugin.hive.containers.HiveMinioDataLake#copyResources to copy files from the test resources to eventually simulate easier your use case.

Thank you for sharing and explaining those..!

I will add some tiny test case on there!

I added tiny test case following your guides :)

cla-bot · 2023-12-20T14:02:29Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

findinpath · 2023-12-21T05:45:30Z

.../trino-hive/src/main/java/io/trino/plugin/hive/procedure/SyncPartitionMetadataProcedure.java

@@ -191,7 +191,7 @@ private static Set<Location> listDirectories(TrinoFileSystem fileSystem, Locatio
    private static String listedDirectoryName(Location directory, Location location)
    {
        String prefix = directory.path();


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 += "/";

}

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

Thanks, I will update the description!

findinpath · 2023-12-21T05:47:14Z

.../trino-hive/src/main/java/io/trino/plugin/hive/procedure/SyncPartitionMetadataProcedure.java

@@ -191,7 +191,7 @@ private static Set<Location> listDirectories(TrinoFileSystem fileSystem, Locatio
    private static String listedDirectoryName(Location directory, Location location)
    {
        String prefix = directory.path();
-        if (!prefix.endsWith("/")) {
+        if (!prefix.endsWith("/") && !prefix.equals("")) {


Suggested change

if (!prefix.endsWith("/") && !prefix.equals("")) {

if (!prefix.isEmpty() && !prefix.endsWith("/")) {

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHive3OnDataLake.java

findinpath · 2023-12-21T05:55:29Z

.../trino-hive/src/main/java/io/trino/plugin/hive/procedure/SyncPartitionMetadataProcedure.java

@@ -191,7 +191,7 @@ private static Set<Location> listDirectories(TrinoFileSystem fileSystem, Locatio
    private static String listedDirectoryName(Location directory, Location location)
    {
        String prefix = directory.path();
-        if (!prefix.endsWith("/")) {


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

Thanks! I am done!

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

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

cla-bot · 2023-12-21T06:17:34Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2023-12-21T06:21:03Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2023-12-21T06:23:43Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

findinpath

LGTM % commit title

findinpath · 2023-12-21T09:33:42Z

@okayhooni i see that the verification/cla-signed is failing.
Pls be so kind to follow https://github.com/trinodb/trino/blob/master/.github/CONTRIBUTING.md

cla-bot · 2023-12-25T09:16:44Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

okayhooni · 2023-12-25T09:23:23Z

@okayhooni i see that the verification/cla-signed is failing. Pls be so kind to follow https://github.com/trinodb/trino/blob/master/.github/CONTRIBUTING.md

@findinpath
I wrote and submitted the CLA to [email protected], just before..!
Thank you for guiding me..! Happy Christmas :)

ebyhr · 2024-01-12T07:07:24Z

@cla-bot check

cla-bot · 2024-01-12T07:07:28Z

The cla-bot has been summoned, and re-checked this pull request!

github-actions bot added tests:hive hive Hive connector labels Dec 13, 2023

okayhooni changed the title ~~Fix bug in listedDirectoryName to handle table located in the root path of s3 bucket~~ Fix bug in sync_partition_metadata() to handle table located in the root path of s3 bucket Dec 13, 2023

findinpath reviewed Dec 19, 2023

View reviewed changes

okayhooni requested a review from findinpath December 20, 2023 14:05

findinpath reviewed Dec 21, 2023

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHive3OnDataLake.java Outdated Show resolved Hide resolved

findinpath reviewed Dec 21, 2023

View reviewed changes

okayhooni changed the title ~~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 Dec 21, 2023

findinpath approved these changes Dec 21, 2023

View reviewed changes

findinpath requested a review from electrum December 21, 2023 09:32

okayhooni force-pushed the hotfix/hive_sync_partition_metadata_proc branch from c5085e8 to 1aa1198 Compare December 25, 2023 09:16

findinpath approved these changes Dec 27, 2023

View reviewed changes

findinpath requested a review from ebyhr January 7, 2024 05:06

cla-bot bot added the cla-signed label Jan 12, 2024

Add handling for root bucket table location in sync_partition_metadata

7f6367f

ebyhr force-pushed the hotfix/hive_sync_partition_metadata_proc branch from 1aa1198 to 7f6367f Compare January 12, 2024 07:51

ebyhr approved these changes Jan 12, 2024

View reviewed changes

ebyhr merged commit ea890c7 into trinodb:master Jan 14, 2024
56 checks passed

github-actions bot added this to the 437 milestone Jan 14, 2024

colebow mentioned this pull request Jan 17, 2024

Add Trino 437 release notes #20404

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add handling for root bucket table location in sync_partition_metadata #20090

Add handling for root bucket table location in sync_partition_metadata #20090

okayhooni commented Dec 13, 2023 •

edited by ebyhr

Loading

cla-bot bot commented Dec 13, 2023

findinpath Dec 19, 2023

okayhooni Dec 20, 2023 •

edited

Loading

findinpath Dec 20, 2023

okayhooni Dec 20, 2023

okayhooni Dec 20, 2023

findinpath Dec 21, 2023

cla-bot bot commented Dec 20, 2023

findinpath Dec 21, 2023

okayhooni Dec 21, 2023

findinpath Dec 21, 2023

findinpath Dec 21, 2023

okayhooni Dec 21, 2023

findinpath Dec 21, 2023

okayhooni Dec 25, 2023

cla-bot bot commented Dec 21, 2023

cla-bot bot commented Dec 21, 2023

cla-bot bot commented Dec 21, 2023

findinpath left a comment

findinpath commented Dec 21, 2023

cla-bot bot commented Dec 25, 2023

okayhooni commented Dec 25, 2023

ebyhr commented Jan 12, 2024

cla-bot bot commented Jan 12, 2024

	public void testPartitionedTableExternalLocationOnTopOfTheBucket()
	{
	String topBucketName = "test-hive-partitioned-top-of-the-bucket-" + randomNameSuffix();
	hiveMinioDataLake.getMinio().createBucket(topBucketName);
	String tableName = "test_external_location_top_of_the_bucket_" + randomNameSuffix();

	assertUpdate(format(
	"CREATE TABLE %s (" +
	" a_varchar varchar, " +
	" pkey integer) " +
	"WITH (" +
	" external_location='%s'," +
	" partitioned_by=ARRAY['pkey'])",
	tableName,
	format("s3://%s/", topBucketName)));
	assertUpdate("INSERT INTO " + tableName + " VALUES ('a', 1) , ('b', 1), ('c', 2), ('d', 2)", 4);
	assertQuery("SELECT * FROM " + tableName, "VALUES ('a', 1), ('b',1), ('c', 2), ('d', 2)");
	assertUpdate("DELETE FROM " + tableName + " where pkey = 2");
	assertQuery("SELECT * FROM " + tableName, "VALUES ('a', 1), ('b',1)");

	assertUpdate("DROP TABLE " + tableName);
	}

	if (!prefix.endsWith("/") && !prefix.equals("")) {
	if (!prefix.isEmpty() && !prefix.endsWith("/")) {

Add handling for root bucket table location in sync_partition_metadata #20090

Add handling for root bucket table location in sync_partition_metadata #20090

Conversation

okayhooni commented Dec 13, 2023 • edited by ebyhr Loading

Description

Release notes

cla-bot bot commented Dec 13, 2023

Choose a reason for hiding this comment

okayhooni Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot bot commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot bot commented Dec 21, 2023

cla-bot bot commented Dec 21, 2023

cla-bot bot commented Dec 21, 2023

findinpath left a comment

Choose a reason for hiding this comment

findinpath commented Dec 21, 2023

cla-bot bot commented Dec 25, 2023

okayhooni commented Dec 25, 2023

ebyhr commented Jan 12, 2024

cla-bot bot commented Jan 12, 2024

okayhooni commented Dec 13, 2023 •

edited by ebyhr

Loading

okayhooni Dec 20, 2023 •

edited

Loading