Skip to content

Conversation

@ZENOTME
Copy link
Contributor

@ZENOTME ZENOTME commented Dec 24, 2024

This PR lets the user indicate to delete the file if the writer doesn't write anything.

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Dec 24, 2024

I haven't add the test for this PR yet because I am not sure whether it's a reasonable design to provide the delete function here. But in practice, it's useful I think. Welcome to any suggestions. cc @liurenjie1024 @Xuanwo

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Feb 27, 2025

I think this PR is ready to review. cc @Xuanwo @liurenjie1024

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @ZENOTME for this pr, instead of adding an extra parameter, should we consider making opening the file lazily, e.g. only create one when we actually append data?

}

/// Delete the file if it exists.
pub async fn delete(&self) -> crate::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a function of FileIO

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Mar 3, 2025

Thanks @ZENOTME for this pr, instead of adding an extra parameter, should we consider making opening the file lazily, e.g. only create one when we actually append data?

Thanks @liurenjie1024. I think it's a good idea. Let me try this

@ZENOTME ZENOTME force-pushed the delete_empty_row_writer branch from 4e07518 to b928c61 Compare March 3, 2025 09:38
@ZENOTME
Copy link
Contributor Author

ZENOTME commented Mar 3, 2025

Hi @liurenjie1024, I use the lazy init to avoid creating new file when empty write. PTAL

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @ZENOTME for fixing this, LGTM!

@liurenjie1024 liurenjie1024 merged commit 2d6fb2b into apache:main Mar 6, 2025
17 checks passed
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.

2 participants