Conversation
smaheshwar-pltr
left a comment
smaheshwar-pltr
left a comment
There was a problem hiding this comment.
Thanks for picking this up @Fokko! Looks great, just small comments 😄
mkdocs/docs/configuration.md
Outdated
| | `write.object-storage.enabled` | Boolean | True | Enables the [`ObjectStoreLocationProvider`](configuration.md#object-store-location-provider) that adds a hash component to file paths. Note: the default value of `True` differs from Iceberg's Java implementation | | ||
| | `write.object-storage.partitioned-paths` | Boolean | True | Controls whether [partition values are included in file paths](configuration.md#partition-exclusion) when object storage is enabled | | ||
| | `write.py-location-provider.impl` | String of form `module.ClassName` | null | Optional, [custom `LocationProvider`](configuration.md#loading-a-custom-location-provider) implementation | | ||
| | `write.data.path` | String pointing to location | ∅ | Sets the location where to write the data. If not set, it will use the table location postfixed with `data/`. | |
There was a problem hiding this comment.
Not sure about the best wording here - maybe something like:
| | `write.data.path` | String pointing to location | ∅ | Sets the location where to write the data. If not set, it will use the table location postfixed with `data/`. | | |
| | `write.data.path` | String referencing a location | ∅ | Sets the location under which data is written. If not set, the table location (`table.metadata.location`) postfixed with `data/` is used. | |
There was a problem hiding this comment.
Also, what about 'table location + /data' as the default instead of ∅ like in the Java docs?
There was a problem hiding this comment.
Also bringing attention to #1537 (comment) - it would be great to make the location providers docs consistent with write.data.path now.
iceberg-python/mkdocs/docs/configuration.md
Line 213 in 4a055d7
iceberg-python/mkdocs/docs/configuration.md
Line 242 in 4a055d7
I think these are the only places that require changing - they'll be write.data.path (and maybe for clarity we should reiterate there that this defaults to {location}/data so examples are clearer). Could we do that in this PR maybe? (Happy to follow-up with this otherwise)
There was a problem hiding this comment.
Thanks for the input here, I've changed some wording en reworked some of the sections. Let me know what you think 👍
There was a problem hiding this comment.
Looks great - thank you!
There was a problem hiding this comment.
LGTM! i added the reference to #1492 in the description
Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com>
3 participants
Relates to #1492