Hive : Fixing the trailing slash issue for the database paths in HMS

[Rajeev-01]

Summary

• While we are creating a new table in hive, if location hasn't passed explicitly we are fetching database path from HMS and appending "/" followed by tableName.
• There are no checks if the HMS db path has trailing slashes.
• This commit fixes a bug where database locations with trailing slashes in Hive Metastore (HMS) would create table locations with double slashes (e.g. database_path//table_name).
• Posted the issue here : Trailing slash issue in Hive Metastore database paths while creating table #14296

Test plan

• Added an UT which will check end to end flow where we will create the db with path having trailing slash and they will be removed while creating the table using hiveCatalaog

[singhpk234]

LGTM, Thanks @Rajeev-01 !

[Rajeev-01]

Thanks @singhpk234 for the approval. All checks are green, can you or someone please merge it

[kevinjqliu]

LGTM

Another potential issue is when setting the "location" value:

iceberg/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

Lines 772 to 773 in 77bc1d0

	} else if (key.equals("location")) {
	database.setLocationUri(value);

I originally thought this was an issue, but its fine since databaseLocation takes care of the trailing slash:

iceberg/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

Lines 725 to 727 in 77bc1d0

	// Otherwise, stick to the {WAREHOUSE_DIR}/{DB_NAME}.db/{TABLE_NAME} path
	String databaseLocation = databaseLocation(tableIdentifier.namespace().levels()[0]);
	return String.format("%s/%s", databaseLocation, tableIdentifier.name());

[huaxingao]

@Rajeev-01 Thanks for the PR. The fix looks good, but seems the test passes without the fix?

[Rajeev-01]

@Rajeev-01 Thanks for the PR. The fix looks good, but seems the test passes without the fix?

Hey yeah @huaxingao thanks for pointing it out, I should have tested it without the fix 😓. The reason for test is not working was the HMS createDatabase call is internally using the org.apache.hadoop.fs.Path for creating the path which normalizes the path by removing the trailing slashes. This will work with hadoop >= 3.x since the fix landed in this version https://issues.apache.org/jira/browse/HADOOP-16226.

[Rajeev-01]

The only way to make it work is by mocking but it's better not to introduce mock in e2e tests.
@huaxingao Can I keep the test as it is or remove it since no use of it or modify it by mocking the dbPath?

[huaxingao]

@Rajeev-01 Thanks for checking this out! Since this is a simple fix, I’m okay merging without a mocked test. I think we can keep the current test as a smoke check (or drop it if others prefer).

[Rajeev-01]

cool thanks @huaxingao
@singhpk234 @pvary @kevinjqliu can you please ack if we are good to have it, if so we can merge it

[ebyhr]

nit: We could use doesNotEndWith method instead.

[ebyhr]

nit: This line can show "Database xxx is not empty" message instead of the failed reason if the above assertion at L126 fails for some reason. We could enable cascade parameter for dropDatabase method.

[kevinjqliu]

Thanks for the fix @Rajeev-01 and thanks everyone for the review.

[aalopatin]

@Rajeev-01 Hi 👋 Does this MR solve this issue? #15908

[Rajeev-01]

@Rajeev-01 Hi 👋 Does this MR solve this issue? #15908

Hey yeah @aalopatin it solves double slash issue for HMS catalog, I thought of this will go in next release 😅

[kevinjqliu]

this has been backported to the 1.10.x (#16010) and will be included when we release 1.10.2 soon