smaheshwar-pltr commented on code in PR #11798:
URL: https://github.com/apache/iceberg/pull/11798#discussion_r1888502532
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -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
Review Comment:
IMHO, this is a tiny bit misleading - although it logically holds that
*"if the flag is set to true and object store is enabled, then partition
values are included"*,
which feels like an equivalent rephrasing, a possible interpretation of the
suggested wording
*"Includes the partition values in the path when set to true and object
store is enabled"*
is that object store must be enabled for this to happen. In other words,
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), this other suggested wording could
be read as "to include partition values, you need to enable this property and
enable object store" - but I don't think that this is correct because [default
location provision includes partition
values](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/LocationProviders.java#L98).
I believe that
```
// With object store enabled, includes the partition values in the path when
set to true
```
could be slightly less misleading, but I actually think that this PR's
suggested wording is the least misunderstandable and also accurately implies
that partition values being included is the default (although I agree that a
comment on when the property is true makes more sense - perhaps in the future
this table property itself could be replaced with its negation).
@ebyhr @amogh-jahagirdar @nastra thank you all a lot 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 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
