Skip to content

Fix file location in case of '/' root#17808

Closed
assaf2 wants to merge 2 commits intotrinodb:masterfrom
assaf2:root-folder-issue
Closed

Fix file location in case of '/' root#17808
assaf2 wants to merge 2 commits intotrinodb:masterfrom
assaf2:root-folder-issue

Conversation

@assaf2
Copy link
Copy Markdown
Member

@assaf2 assaf2 commented Jun 8, 2023

Description

Before this fix, when root was /, the first file name's character was trimmed.
For example when listingLocation="s3a://bucket/", root = "/", path = "/file.parquet" then location turned out to be s3a://bucket/ile.parquet.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( X ) Release notes are required, with the following suggested text:

# Section TBD
* Fix TBD. ({issue}`TBD`)

@cla-bot cla-bot bot added the cla-signed label Jun 8, 2023
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

why not canonicalize the paths in the Location class instead. Seems to be root of all problems?

Right now Location feels more like String.

@assaf2
Copy link
Copy Markdown
Member Author

assaf2 commented Jun 11, 2023

why not canonicalize the paths in the Location class instead. Seems to be root of all problems?

Right now Location feels more like String.

I can change Location#appendPath not to throw in case newPathElement starts with a slash (see implementation on a separate commit), but there are dozens of usages to this method, and I'm not sure if it's ok to do that.

@assaf2 assaf2 requested review from electrum and findepi June 11, 2023 07:45
@assaf2 assaf2 marked this pull request as ready for review June 12, 2023 08:11
* A slash will be added between the current path and the new path element if needed.
*
* @throws IllegalArgumentException if the new path element is empty or starts with a slash
* @throws IllegalArgumentException if the new path element is empty
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should be changing Location.appendPath API.
This can unwanted consequences. More conservative approach would be to keep disallowing leading / here and fix listing instead.

Comment on lines -75 to -76
int index = root.endsWith("/") ? root.length() : root.length() + 1;
Location location = listingLocation.appendPath(path.substring(index));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Restore or we draw from @pajaks 's #17586

@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 12, 2023

I don't think unit testing is sufficient for this kind of problem.
@findinpath is working on end-to-end testing this with Hive connector.
Marius will take prepare a PR with the fix, end-to-end test with with hive connector and also (apparently directly related) VACUUM tests for Delta from #17586.

@findinpath findinpath assigned findinpath and assaf2 and unassigned findinpath Jun 12, 2023
@findinpath
Copy link
Copy Markdown
Contributor

Created #17848 to test reading from Hive partitioned/unpartitioned table having the location on top of the S3 bucket.

@assaf2
Copy link
Copy Markdown
Member Author

assaf2 commented Jun 12, 2023

closing then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants