feat: support shared session for fragment API - #8034
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…che-new # Conflicts: # python/python/tests/test_session.py
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The change fixes append schema inference at the correct boundary: schema lookup and fragment writing now share the complete session, object-store parameters, commit handler, and durable field IDs. Exposing that same session through Python and Java preserves existing no-session behavior while enabling cache reuse.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The change fixes append schema inference at the correct boundary: schema lookup and fragment writing share the complete session, object-store parameters, commit handler, and durable field IDs. Exposing the same session through Python and Java preserves existing no-session behavior while enabling cache reuse.
| } | ||
|
|
||
| async fn existing_dataset(&self, params: &WriteParams) -> Result<Option<Dataset>> { | ||
| match self.load_existing_dataset(params).await { |
There was a problem hiding this comment.
When no schema is given, FragmentCreateBuilder::write loads the existing dataset to infer the schema (with correct field ids). Previously this load built a bare DatasetBuilder that ignored WriteParams.session, so it always bypassed the shared session's metadata cache. Now it goes through load_existing_dataset, which passes the session (plus store params / commit handler) into ReadParams, so the manifest load hits the shared cache.
| Ok(None) | ||
| } | ||
| let params = self.write_params.map(Cow::Borrowed).unwrap_or_default(); | ||
| match self.load_existing_dataset(¶ms).await { |
There was a problem hiding this comment.
feels like we just repeat the logic here?
There was a problem hiding this comment.
The shared loading logic is already extracted into load_existing_dataset; only the error handling is kept separate on purpose, because the two callers tolerate different errors:
existing_dataset_schemaonly swallowsDatasetNotFoundexisting_datasetswallows bothDatasetNotFoundandNotFound
Merging them would change behavior — e.g. schema inference would silently fall back to the reader schema on a missing manifest file (NotFound) instead of surfacing the error. To keep this PR behavior-compatible, I kept the match arms as they were before.
Closes: #7974