Decode paths as URIs in delta-lake connector #15201

jkylling · 2022-11-25T19:58:38Z

Description

The sample uri delta table was obtained by copying databricks/bar and changing the name of the parquet files and modifying the log entries.

Additional context and related issues

Fixes #15183

Release notes

(x) Release notes are required, with the following suggested text:

# Delta Lake
* Fix failure when the path contains special characters. ({issue}`15183`)

cla-bot · 2022-11-25T19:58:41Z

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 cla@trino.io. 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 · 2022-11-25T20:26:24Z

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 cla@trino.io. 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

findepi · 2022-11-25T22:28:45Z

...rino-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeConnectorSmokeTest.java

+    public void testPathUriDecoding()
+    {
+        String tableName = "test_uri_table_" + randomNameSuffix();
+        registerTableFromResources(tableName, "databricks/uri", getQueryRunner());


The test doesn't prove the Delta change is correct impl of Delta spec, since it can be both our writer and our reader are wrong.

@ebyhr do you know which test class would be the best for this type of a test?
we have a few TestDeltaLakeDatabricks*Compatibility, but none seem applicable to me

I can't find a good existing class either. Adding a new product test class looks good to me.

@jkylling Can you try adding a product test? Let us know if you need help.

There is a bit of test coverage of this (as witnessed by the test failures in 1ca31c1). The new test added only uses the reader, and would fail with the old implementation.

I've tried adding a product test in 7a51388

ebyhr · 2022-12-09T00:49:34Z

Rebased on upstream since check-commits-dispatcher has been failing.

ebyhr · 2022-12-09T00:51:34Z

/test-with-secrets sha=df6138fba5b3544dbeb092752c05c6cde457798f

...c/main/java/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksInsertCompatibility.java

...rino-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeConnectorSmokeTest.java

...c/main/java/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksInsertCompatibility.java

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSplitManager.java

...c/main/java/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksInsertCompatibility.java

github-actions · 2022-12-09T03:41:22Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/3653870944

ebyhr

Looks good except for comments. Please squash commits into one.

...rino-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeConnectorSmokeTest.java

...c/main/java/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksSelectCompatibility.java

ebyhr · 2022-12-10T00:59:43Z

The new test is failing on OSS Delta Lake.

2022-12-09T21:16:22.1791177Z tests               | 2022-12-10 03:01:22 INFO: FAILURE     /    io.trino.tests.product.deltalake.TestDeltaLakeDatabricksSelectCompatibility.testPartitionedSelectSpecialCharacters (Groups: profile_specific_tests, delta-lake-databricks, delta-lake-oss) took 9.0 seconds
2022-12-09T21:16:22.1792228Z tests               | 2022-12-10 03:01:22 SEVERE: Failure cause:
2022-12-09T21:16:22.1792737Z tests               | java.lang.AssertionError: Expected row count to be <1>, but was <0>; rows=[]
2022-12-09T21:16:22.1794078Z tests               | 	at io.trino.tests.product.deltalake.TestDeltaLakeDatabricksSelectCompatibility.testPartitionedSelectSpecialCharacters(TestDeltaLakeDatabricksSelectCompatibility.java:69)
2022-12-09T21:16:22.1795317Z tests               | 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2022-12-09T21:16:22.1796063Z tests               | 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
2022-12-09T21:16:22.1796969Z tests               | 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2022-12-09T21:16:22.1797550Z tests               | 	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
2022-12-09T21:16:22.1798212Z tests               | 	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
2022-12-09T21:16:22.1798875Z tests               | 	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
2022-12-09T21:16:22.1799471Z tests               | 	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
2022-12-09T21:16:22.1800058Z tests               | 	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
2022-12-09T21:16:22.1800617Z tests               | 	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
2022-12-09T21:16:22.1801281Z tests               | 	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
2022-12-09T21:16:22.1804805Z tests               | 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
2022-12-09T21:16:22.1805688Z tests               | 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
2022-12-09T21:16:22.1806301Z tests               | 	at java.base/java.lang.Thread.run(Thread.java:833)

jkylling · 2022-12-10T11:59:19Z

The new test is failing on OSS Delta Lake.

2022-12-09T21:16:22.1791177Z tests               | 2022-12-10 03:01:22 INFO: FAILURE     /    io.trino.tests.product.deltalake.TestDeltaLakeDatabricksSelectCompatibility.testPartitionedSelectSpecialCharacters (Groups: profile_specific_tests, delta-lake-databricks, delta-lake-oss) took 9.0 seconds
2022-12-09T21:16:22.1792228Z tests               | 2022-12-10 03:01:22 SEVERE: Failure cause:
2022-12-09T21:16:22.1792737Z tests               | java.lang.AssertionError: Expected row count to be <1>, but was <0>; rows=[]
2022-12-09T21:16:22.1794078Z tests               | 	at io.trino.tests.product.deltalake.TestDeltaLakeDatabricksSelectCompatibility.testPartitionedSelectSpecialCharacters(TestDeltaLakeDatabricksSelectCompatibility.java:69)
2022-12-09T21:16:22.1795317Z tests               | 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2022-12-09T21:16:22.1796063Z tests               | 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
2022-12-09T21:16:22.1796969Z tests               | 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2022-12-09T21:16:22.1797550Z tests               | 	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
2022-12-09T21:16:22.1798212Z tests               | 	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
2022-12-09T21:16:22.1798875Z tests               | 	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
2022-12-09T21:16:22.1799471Z tests               | 	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
2022-12-09T21:16:22.1800058Z tests               | 	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
2022-12-09T21:16:22.1800617Z tests               | 	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
2022-12-09T21:16:22.1801281Z tests               | 	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
2022-12-09T21:16:22.1804805Z tests               | 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
2022-12-09T21:16:22.1805688Z tests               | 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
2022-12-09T21:16:22.1806301Z tests               | 	at java.base/java.lang.Thread.run(Thread.java:833)

The $path syntax is not supported on Spark, and it was even escaped in ", so we got the string literal '$path'. I've changed it to use input_file_name. However, I tested this on Databricks, and it looks like input_file_name returns the URI encoded path (created delta-io/delta#1517 for this), so the test will likely continue to fail. We can probably skip comparing with the filename obtained from Spark here, and only check that Trino paths are internally compatible?

ebyhr

Please remove .crc files.

...t-tests/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeSelectCompatibility.java

ebyhr · 2022-12-12T00:06:58Z

/test-with-secrets sha=b63a9780e3174e4de3a96bdd1f5b48c76d4acc19

github-actions · 2022-12-12T00:24:39Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/3671461022

ebyhr · 2022-12-12T00:34:39Z

I will investigate TestDeltaLakeAdlsConnectorSmokeTest.testPathUriDecoding failure.

ebyhr · 2022-12-13T02:36:39Z

/test-with-secrets sha=edb11bea6c15a548e80a85dd5dedb4c176e80067
https://github.com/trinodb/trino/actions/runs/3681717402

github-actions · 2022-12-13T03:40:24Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/3681717402

findepi reviewed Nov 25, 2022

View reviewed changes

ebyhr mentioned this pull request Nov 30, 2022

Don't run trino-faulttolerant-tests when PR changes unrelate to the module #15248

Closed

github-actions bot added the tests:hive label Dec 8, 2022

ebyhr force-pushed the fix-trino-uri branch from d641cbb to df6138f Compare December 9, 2022 00:49

cla-bot bot added the cla-signed label Dec 9, 2022

ebyhr reviewed Dec 9, 2022

View reviewed changes

...c/main/java/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksInsertCompatibility.java Outdated Show resolved Hide resolved

ebyhr reviewed Dec 10, 2022

View reviewed changes

trinodb deleted a comment from cla-bot bot Dec 10, 2022

ebyhr reviewed Dec 10, 2022

View reviewed changes

...t-tests/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeSelectCompatibility.java Show resolved Hide resolved

...t-tests/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeSelectCompatibility.java Outdated Show resolved Hide resolved

ebyhr force-pushed the fix-trino-uri branch from d68c2ad to b63a978 Compare December 12, 2022 00:06

Decode path as URI in Delta Lake connector

edb11be

ebyhr force-pushed the fix-trino-uri branch from b63a978 to edb11be Compare December 13, 2022 01:53

ebyhr merged commit 58bd159 into trinodb:master Dec 13, 2022

ebyhr mentioned this pull request Dec 13, 2022

Release notes for 405 #15058

Closed

github-actions bot added this to the 404 milestone Dec 13, 2022

colebow mentioned this pull request Dec 14, 2022

Add Trino 405 release notes #15139

Merged

findepi mentioned this pull request Dec 21, 2022

Fix failure when path contains special character on ADLS in Delta Lake #15381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decode paths as URIs in delta-lake connector #15201

Decode paths as URIs in delta-lake connector #15201

jkylling commented Nov 25, 2022 •

edited by ebyhr

Loading

cla-bot bot commented Nov 25, 2022

cla-bot bot commented Nov 25, 2022

findepi Nov 25, 2022

ebyhr Nov 30, 2022 •

edited

Loading

jkylling Dec 8, 2022

ebyhr commented Dec 9, 2022

ebyhr commented Dec 9, 2022

github-actions bot commented Dec 9, 2022

ebyhr left a comment

ebyhr commented Dec 10, 2022

jkylling commented Dec 10, 2022 •

edited

Loading

ebyhr left a comment

ebyhr commented Dec 12, 2022

github-actions bot commented Dec 12, 2022

ebyhr commented Dec 12, 2022 •

edited

Loading

ebyhr commented Dec 13, 2022 •

edited

Loading

github-actions bot commented Dec 13, 2022

Decode paths as URIs in delta-lake connector #15201

Decode paths as URIs in delta-lake connector #15201

Conversation

jkylling commented Nov 25, 2022 • edited by ebyhr Loading

Description

Additional context and related issues

Release notes

cla-bot bot commented Nov 25, 2022

cla-bot bot commented Nov 25, 2022

findepi Nov 25, 2022

Choose a reason for hiding this comment

ebyhr Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

jkylling Dec 8, 2022

Choose a reason for hiding this comment

ebyhr commented Dec 9, 2022

ebyhr commented Dec 9, 2022

github-actions bot commented Dec 9, 2022

ebyhr left a comment

Choose a reason for hiding this comment

ebyhr commented Dec 10, 2022

jkylling commented Dec 10, 2022 • edited Loading

ebyhr left a comment

Choose a reason for hiding this comment

ebyhr commented Dec 12, 2022

github-actions bot commented Dec 12, 2022

ebyhr commented Dec 12, 2022 • edited Loading

ebyhr commented Dec 13, 2022 • edited Loading

github-actions bot commented Dec 13, 2022

jkylling commented Nov 25, 2022 •

edited by ebyhr

Loading

ebyhr Nov 30, 2022 •

edited

Loading

jkylling commented Dec 10, 2022 •

edited

Loading

ebyhr commented Dec 12, 2022 •

edited

Loading

ebyhr commented Dec 13, 2022 •

edited

Loading