Skip to content

Throw on {write.folder-storage.path,write.object-storage.path} properties #12315

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

Merged
merged 8 commits into from
Apr 9, 2025

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Feb 18, 2025

Another way of warning non-Java users about the deprecated properties

  • write.folder-storage.path
  • write.object-storage.path

Has been deprecated since 0.13.0 in #3094

…rties

Another way of warning non-Java users about the deprecated properties

- `write.folder-storage.path`
- `write.object-storage.path`

This will only warn when `write.data.path` is not set. So hopefuly
the number in the logs will be limited.
@Fokko Fokko force-pushed the fd-warn-on-deprecations branch from 65ec291 to 6b8db34 Compare February 19, 2025 13:10
@Fokko Fokko force-pushed the fd-warn-on-deprecations branch from 6b8db34 to 216a04a Compare February 19, 2025 13:37
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

I wonder whether the now "illegal" properties should also be caught when setting namespace/table/view properties, so that it becomes impossible to set those properties (and run into hard failures when accessing data).

There seem to be more usages of OBJECT_STORE_PATH and WRITE_FOLDER_STORAGE_LOCATION - e.g. IcebergToGlueConverter and RewriteTablePathUtil

@Fokko Fokko changed the title Warn on {write.folder-storage.path,write.object-storage.path} properties Throw on {write.folder-storage.path,write.object-storage.path} properties Feb 19, 2025
@Fokko
Copy link
Contributor Author

Fokko commented Feb 19, 2025

There seem to be more usages of OBJECT_STORE_PATH and WRITE_FOLDER_STORAGE_LOCATION - e.g. IcebergToGlueConverter and RewriteTablePathUtil

I think we can just remove them with 2.0, where they are used it is harmless (removing them when you copy a table, rewriting them when you write the base location, etc).

@Fokko Fokko force-pushed the fd-warn-on-deprecations branch from ba0871f to 2ae5fca Compare February 19, 2025 16:11
@snazy
Copy link
Member

snazy commented Feb 19, 2025

I wonder whether the now "illegal" properties should also be caught when setting namespace/table/view properties, so that it becomes impossible to set those properties (and run into hard failures when accessing data).

I think that would quite useful to prevent users from putting in any of the "illegal" properties. WDYT?

@Fokko
Copy link
Contributor Author

Fokko commented Feb 19, 2025

I think that would quite useful to prevent users from putting in any of the "illegal" properties. WDYT?

I'm not a fan of it, because I think it is very hard to enforce across implementations. For example, we don't support the ones that were deprecated in this PR in PyIceberg/Rust/Go, let other vendors. Since we need to maintain this list, we're also not able to completely remove them at some point.

@Fokko Fokko force-pushed the fd-warn-on-deprecations branch from 16fa9b6 to bdd63f5 Compare February 19, 2025 19:19
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Mar 28, 2025
@github-actions github-actions bot removed the stale label Apr 3, 2025
@Fokko Fokko merged commit b15f4ac into apache:main Apr 9, 2025
42 checks passed
@Fokko Fokko deleted the fd-warn-on-deprecations branch April 30, 2025 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants