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.

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

[feature] Add support for write.data.path and write.metadata.path #1492

Closed
2 tasks done
kevinjqliu opened this issue Jan 7, 2025 · 12 comments
Closed
2 tasks done

[feature] Add support for write.data.path and write.metadata.path #1492

kevinjqliu opened this issue Jan 7, 2025 · 12 comments
Assignees

Comments

@kevinjqliu
Copy link
Contributor

kevinjqliu commented Jan 7, 2025

Feature Request / Improvement

In the write properties section

#1452 adds support for LocationProvider #1452 (comment)
Similar issue, #161

@jiakai-li
Copy link
Contributor

jiakai-li commented Jan 7, 2025

Sorry I raised a duplicated one by accident, which is closed now. Can I work on this feature?

@kevinjqliu
Copy link
Contributor Author

kevinjqliu commented Jan 7, 2025

@jiakai-li assigned to you!

@kevinjqliu
Copy link
Contributor Author

BTW this is how java side is done https://grep.app/search?q=WRITE_DATA_LOCATION&filter[repo][0]=apache/iceberg&filter[path][0]=core/src/

@smaheshwar-pltr
Copy link
Contributor

Thanks for volunteering @jiakai-li! Happy to review the LocationProvider-related changes for write.data.path if it'd help 😄

@jiakai-li
Copy link
Contributor

Hey @smaheshwar-pltr , thanks for the offer! I hadn’t realized this feature is closely tied to LocationProvider. After looking into it, I think it could fit well with the changes you’re already working on. Would you like to handle it as part of your update, or would you prefer I continue working on it?

@smaheshwar-pltr
Copy link
Contributor

I hadn’t realized this feature is closely tied to LocationProvider. After looking into it, I think it could fit well with the changes you’re already working on. Would you like to handle it as part of your update, or would you prefer I continue working on it?

Good suggestion @jiakai-li - I expressed my thoughts here: #1452 (comment). I'd prefer to keep #1452 as just what's needed to address the issue linked on it because:

I'd prefer that both of these new write options be tackled together in a follow-up PR, that closes this issue - let me know how this sounds to you.

@jiakai-li
Copy link
Contributor

Sure, that makes sense to me, thank you @smaheshwar-pltr , :-)

@jiakai-li jiakai-li removed their assignment Jan 9, 2025
@kevinjqliu
Copy link
Contributor Author

#1452 is merged, we can now work on adding support for the above

@Fokko
Copy link
Contributor

Fokko commented Jan 31, 2025

I'm also happy to pick this up!

Fokko added a commit that referenced this issue Feb 7, 2025
Relates to #1492

---------

Co-authored-by: Kevin Liu <[email protected]>
@geruh
Copy link
Contributor

geruh commented Feb 7, 2025

I'd like to pick up the write.metadata.path impl if no one has already!

@kevinjqliu
Copy link
Contributor Author

sure @geruh thank you!

kevinjqliu added a commit that referenced this issue Feb 14, 2025
Adding support for writing metadata to a custom path set via
`write.metadata.path` property. Since the Python library consolidates
the table operation classes in both the table and catalog classes, I had
to surface the metadata file location handling to the base `Catalog`
class to avoid circular dependencies. This way we are also able to
centralize the metadata location handling for table metadata and
snapshots.


Relates to #1492

---------

Co-authored-by: Kevin Liu <[email protected]>
Co-authored-by: Fokko Driesprong <[email protected]>
Co-authored-by: smaheshwar-pltr <[email protected]>
@kevinjqliu
Copy link
Contributor Author

All done!

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

No branches or pull requests

5 participants