feat(java): support credential vending at write time #5309
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| /** | ||
| * Sets whether to ignore storage options from the namespace's describe_table() or |
There was a problem hiding this comment.
Should we use DescribeTable/describeTable and CreateEmptyTable/createEmptyTable?
| /** | ||
| * Sets the schema for the dataset. | ||
| * | ||
| * <p>If not provided, the schema will be inferred from the reader. |
There was a problem hiding this comment.
I guess the comment should be 'If the reader and stream not provided, use the schema to create an empty dataset'? (get from code below)
| private void validate() { | ||
| Preconditions.checkNotNull(datasetUri, "datasetUri is required"); | ||
| Preconditions.checkState( | ||
| vectorSchemaRoot != null || arrowArrayStream != null, |
There was a problem hiding this comment.
If vectorSchemaRoot != null && arrowArrayStream != null, it seems arrowArrayStream would be ignored. Shall we validate it?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Build WriteParams with merged storage options | ||
| WriteParams.Builder paramsBuilder = | ||
| new WriteParams.Builder().withMode(mode).withStorageOptions(mergedStorageOptions); | ||
|
|
||
| maxRowsPerFile.ifPresent(paramsBuilder::withMaxRowsPerFile); |
There was a problem hiding this comment.
In the namespace write path the builder merges namespace storage options and builds WriteParams (lines 387‑391) but never attaches a StorageOptionsProvider the way OpenDatasetBuilder does for reads. When a namespace vend temporary credentials (e.g., with a short expires_at_millis), long-running writes through this builder will keep using the initial, soon-to-expire options and cannot refresh them, causing write failures once the credentials expire despite the new credential‑vending support.
Useful? React with 👍 / 👎.
majin1102
left a comment
majin1102
left a comment
There was a problem hiding this comment.
Looks good to me, left a nit comment
java/lance-jni/src/utils.rs
Outdated
| extract_storage_options(env, storage_options_obj)?; | ||
|
|
||
| // Extract storage options provider if present | ||
| let storage_options_provider = if !storage_options_provider_obj.is_null() { |
There was a problem hiding this comment.
nit: It's fine to extract optional struct like this. I think this util method could make it simpler:
lance/java/lance-jni/src/ffi.rs
Lines 139 to 141 in 2e21599
jackye1995
force-pushed
the
java-vending-write
branch
from
f81547c to
a58dbd5
Compare
2 participants
No description provided.