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

Jump to bottom

Add ability to have unique table location for each delta lake table #13020

Merged
merged 1 commit into from
Jul 25, 2022
Merged

Add ability to have unique table location for each delta lake table #13020

merged 1 commit into from
Jul 25, 2022

Conversation

homar
Copy link
Member

@homar homar commented Jun 28, 2022 •
edited by ebyhr
Loading

Description

Is this change a fix, improvement, new feature, refactoring, or other?

improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

a connector

How would you describe this change to a non-technical end user or system administrator?

each table will have different location random location, unique suffix will be added to the table name

Related issues, pull requests, and links

#12980

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Delta Lake
* Add ability to have unique table location for each delta lake table  ({[issue](https://github.com/trinodb/trino/issues/12980)}`12980`)

@homar homar force-pushed the homar/generate_unique_location_when_delta_table_created_with_no_explicit_location branch from 48e56e9 to 49f0141 Compare June 28, 2022 14:22
@cla-bot cla-bot bot added the cla-signed label Jun 28, 2022
@homar homar force-pushed the homar/generate_unique_location_when_delta_table_created_with_no_explicit_location branch 4 times, most recently from c993373 to a5c9e0e Compare June 29, 2022 16:01
@homar homar changed the title [WIP] Add ability to have unique table location for each delta lake table Add ability to have unique table location for each delta lake table Jun 29, 2022
@homar homar force-pushed the homar/generate_unique_location_when_delta_table_created_with_no_explicit_location branch 12 times, most recently from 47fec5d to b4fa327 Compare July 1, 2022 14:27
@homar homar force-pushed the homar/generate_unique_location_when_delta_table_created_with_no_explicit_location branch from b4fa327 to 4f37402 Compare July 15, 2022 13:11
@homar homar requested review from findepi, alexjo2144 and ebyhr July 18, 2022 13:07
@findepi
Copy link
Member

findepi commented Jul 21, 2022

is the ci failure related?

@homar
Copy link
Member Author

homar commented Jul 21, 2022

is the ci failure related?

no it is not

@findepi
Copy link
Member

findepi commented Jul 21, 2022

@alexjo2144 @ebyhr ptal

no it is not

if it's a flake, is there an issue already?

@ebyhr
Copy link
Member

ebyhr commented Jul 21, 2022

I believe the test was already removed in 308e717.

@homar homar force-pushed the homar/generate_unique_location_when_delta_table_created_with_no_explicit_location branch from 4f37402 to 796b8b1 Compare July 22, 2022 07:53
@homar
Copy link
Member Author

homar commented Jul 22, 2022

@ebyhr comments applied

@homar homar force-pushed the homar/generate_unique_location_when_delta_table_created_with_no_explicit_location branch from 796b8b1 to 9bc82f9 Compare July 22, 2022 08:47
@ebyhr ebyhr mentioned this pull request Jul 22, 2022
Closed
@ebyhr ebyhr force-pushed the homar/generate_unique_location_when_delta_table_created_with_no_explicit_location branch from 9bc82f9 to cfb6ba2 Compare July 23, 2022 07:35
@ebyhr
Copy link
Member

ebyhr commented Jul 23, 2022

Rebased on upstream to resolved conflicts.

@homar homar force-pushed the homar/generate_unique_location_when_delta_table_created_with_no_explicit_location branch 3 times, most recently from 9bc82f9 to 2ba95c4 Compare July 24, 2022 18:20
@homar
Copy link
Member Author

homar commented Jul 24, 2022

@ebyhr can we merge this ?

@ebyhr
Copy link
Member

ebyhr commented Jul 25, 2022

@homar TestDeltaLakeAdlsConnectorSmokeTest#testManagedTableFilesCleanedOnDrop looks failing consistently (https://github.com/trinodb/trino/runs/7481219750 & in my laptop). Could you take a look at that?

Error:  io.trino.plugin.deltalake.TestDeltaLakeAdlsConnectorSmokeTest.testManagedTableFilesCleanedOnDrop  Time elapsed: 1.97 s  <<< FAILURE!
java.lang.AssertionError: 
Expecting:
 <0>
to be greater than:
 <0> 
	at io.trino.plugin.deltalake.BaseDeltaLakeConnectorSmokeTest.testManagedTableFilesCleanedOnDrop(BaseDeltaLakeConnectorSmokeTest.java:713)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)

@homar homar force-pushed the homar/generate_unique_location_when_delta_table_created_with_no_explicit_location branch from 2ba95c4 to 476d685 Compare July 25, 2022 09:16
@homar
Copy link
Member Author

homar commented Jul 25, 2022

@ebyhr I pushed a fix for ADSL, could you please run ADSL tests again ?

@ebyhr
Copy link
Member

ebyhr commented Jul 25, 2022

Sure, updated #13310

@homar
Copy link
Member Author

homar commented Jul 25, 2022 •
edited
Loading

@ebyhr I think now we are good to merge, if my last change to fix that adsl test is fine for you

@findepi findepi requested a review from ebyhr July 25, 2022 15:14
@ebyhr ebyhr merged commit 2c7db33 into trinodb:master Jul 25, 2022
@ebyhr
Copy link
Member

ebyhr commented Jul 25, 2022

Merged, thanks!

@github-actions github-actions bot added this to the 392 milestone Jul 25, 2022
@ebyhr ebyhr mentioned this pull request Jul 25, 2022
Closed
alexjo2144
alexjo2144 reviewed Jul 25, 2022
View reviewed changes
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java
boolean requiresOptIn = transactionLogWriterFactory.newWriter(session, tableHandle.getLocation()).isUnsafe();
String tableLocation = metastore.getTableLocation(tableHandle.getSchemaTableName(), session);
Path tableMetadataDirectory = new Path(new Path(tableLocation).getParent().toString(), tableHandle.getTableName());
boolean requiresOptIn = transactionLogWriterFactory.newWriter(session, tableMetadataDirectory.toString()).isUnsafe();
Copy link
Member

Choose a reason for hiding this comment

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

Why was this change needed?

@colebow colebow mentioned this pull request Jul 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexjo2144 alexjo2144 alexjo2144 left review comments

@findepi findepi findepi approved these changes

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed docs
Milestone
392
Development

Successfully merging this pull request may close these issues.
4 participants
@homar @findepi @ebyhr @alexjo2144