Skip to content

Conversation

vlovich
Copy link

@vlovich vlovich commented Aug 6, 2025

Android doesn't support hard_link so provide an alternate path for performing the requested operation.

Which issue does this PR close?

Closes #459.

Rationale for this change

On Android hard links are not allowed. However, there are alternate ways to express atomicity of the same operations - we use OpenOptions::create_new instead to guarantee the target exists or not before clobbering it.

What changes are included in this PR?

LocalFilesystem put/copy/copy_if_not_exists alternatives for Android that don't use hard_link.

Are there any user-facing changes?

No.

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

This will briefly create an empty file which could be read by a racing reader, violating atomicity

@vlovich vlovich force-pushed the android-support branch 2 times, most recently from 2d1fcea to 6156595 Compare August 6, 2025 17:23
@vlovich
Copy link
Author

vlovich commented Aug 6, 2025

I've addressed the comment by switching the implementation to use renameat2 with the RENAME_NOREPLACE. copy_if_not_exists required a little bit more complicated implementation since it has to go through a staging file & do it in a loop so that it had something to rename, so I made separate implementation android / not android implementations.

I believe this addresses all the comments and gets this working on Android?

Android doesn't support hard_link so provide an alternate path for
performing the requested operation.

This also fixes a bug where failed put might result in the staged file
being left around.
@tustvold
Copy link
Contributor

tustvold commented Aug 6, 2025

Is there some way we can test this? I presume we could add some build flag or something so we could test in CI on Linux without it becoming a public feature?

I will also add that I am not sure what android versions will support this, it is on the newer end of Linux features.

@vlovich
Copy link
Author

vlovich commented Aug 8, 2025

FWIW according to docs renameat2 is available from Linux 3.15 for ext4 so support for it is actually quite old I think (ext4 is the FS that Android tends to use).

Can we merge this as is? I tested it by hand via lancedb and it works. I recognize there is some risk but I think that can be fixed by future commits & the risk is mitigated by this being configured out for non-Android platforms.

@tustvold
Copy link
Contributor

tustvold commented Aug 8, 2025

I'm not comfortable merging code without some means of testing it, but it should be possible to construct something using a cfg flag or similar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fails to create table on Android
2 participants