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

Fix comment on WRITE_OBJECT_STORE_PARTITIONED_PATHS table property #11798

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

smaheshwar-pltr
Copy link

The code comment above the WRITE_OBJECT_STORE_PARTITIONED_PATHS constant in TableProperties was incorrect - partition values are excluded when this property is set to false, not true, and object store is enabled, as stated in the docs. This PR fixes the comment to be consistent.

@github-actions github-actions bot added the core label Dec 16, 2024
@ebyhr
Copy link
Contributor

ebyhr commented Dec 16, 2024

I believe this change is correct. The usage is:

this.includePartitionPaths =
PropertyUtil.propertyAsBoolean(
properties,
TableProperties.WRITE_OBJECT_STORE_PARTITIONED_PATHS,
TableProperties.WRITE_OBJECT_STORE_PARTITIONED_PATHS_DEFAULT);

@@ -244,7 +244,7 @@ private TableProperties() {}
public static final String OBJECT_STORE_ENABLED = "write.object-storage.enabled";
public static final boolean OBJECT_STORE_ENABLED_DEFAULT = false;

// Excludes the partition values in the path when set to true and object store is enabled
// Excludes the partition values in the path when set to false and object store is enabled
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Dec 17, 2024

Choose a reason for hiding this comment

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

Nit: While this comment is correct, I feel like having the comment when the property is true reads a little bit more easy.

// Includes the partition values in the path when set to true and object store is enabled

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to changing the comment as @amogh-jahagirdar suggested

Copy link
Author

@smaheshwar-pltr smaheshwar-pltr Dec 17, 2024

Choose a reason for hiding this comment

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

Hmm, I feel like this suggested wording could be misinterpreted as "object store must be enabled for partition values to be included" (but default location provision includes partition values), just as this PR's proposed wording could be read as, "to exclude partition values, you should disable this property and enable object store" (which is correct, modulo custom location provision).

I agree that a comment on when the property is true reads better but this PR's wording seems the least misunderstandable IMHO.

@ebyhr @amogh-jahagirdar @nastra thank you for reviewing - let me know your thoughts! Happy to be overruled here and make the suggested change if I'm being too pedantic, or to take other suggestions 😄

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 Jan 17, 2025
@smaheshwar-pltr smaheshwar-pltr force-pushed the smaheshwar/fix-partition-docstring branch from f4536aa to 0bb2eb3 Compare January 17, 2025 14:29
@smaheshwar-pltr
Copy link
Author

Any update on this?

Happy to change if #11798 (comment) is baseless. I think fixing a misleading comment would be an improvement though.

@github-actions github-actions bot removed the stale label Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants