Repository navigation
Conversation
A cache miss records the object etag and then reads the bytes. The GET now requires that etag, including when `s3_validate_etag_on_read` or `azure_validate_etag_on_read` is off, so an in-place overwrite is not stored under the old key. A weak etag is not used as a cache key. Co-authored-by: Cursor <cursoragent@cursor.com>
Omitting `expected_cardinality` skipped `PUFFIN_DV_MAX_MATERIALIZED_POSITIONS`. `deserializeRoaringPositionBitmap` now rejects a bitmap above that ceiling before expanding it into positions. Co-authored-by: Cursor <cursoragent@cursor.com>
`PuffinFilesCache` hid non-virtual `clear`, `setMaxSizeInBytes`, and `setMaxCount`, so a `CacheBase` pointer left the footer memo in place. Those methods are now virtual. Co-authored-by: Cursor <cursoragent@cursor.com>
A DAG that only adds columns has no inputs. The cluster protocol treated that as a missing `schema_transform` and dropped it. An empty DAG remains the sentinel for no transform. Co-authored-by: Cursor <cursoragent@cursor.com>
Bucket splitting copies through `clone`. `ObjectInfoInArchive` and `ObjectStorageQueueObjectInfo` fell back to the base copy and dropped the archive identity and `file_metadata`. Co-authored-by: Cursor <cursoragent@cursor.com>
A mismatch used to retry without the pin and could apply a later object under the listed offset, length, and cardinality. `getOrSetDeletionVector` already skips the insert when the loader throws. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The debug build treats a missing prototype as an error, so the cluster read-task helper cannot stay at file scope. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
CI triage for afa617e5 failing checks, 0 caused by this PR. Every one of them fails the same way on other recent PRs into
Details1. This is an access-control problem with the test's named collection or user config. Your diff doesn't touch it, and #2704 fails with exactly the same message. 2. This comes from the S3 write path. The PR only adds a unit test for 3. #2704 fails the same way. 4. Regression oauth / swarms: both runs fail on exactly the same scenarios as the runs for #2700 (certificate backport) and #2680, which don't touch this code. Swarms deserved a closer look because the PR changes About the RecommendationThe PR is fine as far as CI can tell. Merging is safe from a CI point of view, or rerun the failed jobs if you want a cleaner signal. Two of the base-branch failures should get their own tickets: the Evidence: praktika |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Iceberg Puffin deletion-vector cache reads are pinned to the listed etag, including when
s3_validate_etag_on_readorazure_validate_etag_on_readis off. A changed object fails the query instead of being applied under the listed offset, length, and cardinality. Materialization stays withinPUFFIN_DV_MAX_MATERIALIZED_POSITIONSwhen cardinality is omitted.PuffinFilesCacheclear,setMaxSizeInBytes, andsetMaxCountupdate the footer memo throughCacheBase. Cluster reads keep an output-onlyschema_transform. Archive and queue objects keep their type inObjectInfo::clone.Documentation entry for user-facing changes
No new setting or syntax. With the Puffin files cache enabled, a deletion-vector read is tied to the etag from the listing. S3 and Azure send that etag as
If-Matcheven when etag validation is disabled. Local, HDFS, and Web compare the etag after the read. A mismatch raises an error and is not stored in the cache. An empty or weak etag is not used as a cache key. A deletion vector is rejected when it would materialize more than 100000000 positions, whether or not a cardinality was declared.SYSTEM DROP ...cache clear and a runtime cache resize drop memoized Puffin footers together with cached deletion vectors. A distributed read keeps a schema evolution transform that only adds columns. Bucket splitting no longer drops the archive path or the queue file metadata.CI/CD Options
Exclude tests:
Regression jobs to run:
Title: Pin Puffin deletion-vector reads and preserve cluster schema and object identity
Motivation
A Puffin deletion-vector cache miss recorded the object etag and then read the bytes without requiring that etag when
s3_validate_etag_on_readorazure_validate_etag_on_readwas off. An in-place overwrite could be stored under the old key. Local, HDFS, and Web ignoreStoredObject::etag, so the same race existed there even with validation enabled. A first version of the pin retried the read without the etag, which could apply the new object under the listed offset, length, and cardinality.Two related limits were incomplete. Omitting
expected_cardinalityskippedPUFFIN_DV_MAX_MATERIALIZED_POSITIONSand let a compact roaring bitmap expand without a ceiling.PuffinFilesCache::clear,setMaxSizeInBytes, andsetMaxCounthid the non-virtualCacheBasemethods, so a base pointer left the footer memo resident.On the cluster read path, a
schema_transformthat only adds columns has outputs and no inputs. Deserialize treated that the same as the empty DAG used when there is no transform, and the old protocol did not reject it. Bucket splitting copies each object throughObjectInfo::clone.IcebergDataObjectInfoalready copied itself.ObjectInfoInArchiveandObjectStorageQueueObjectInfofell back to the base copy and lost the archive path, reader, and file info, or the queuefile_metadata.Approach
The cache-miss load passes the listed etag into
createReadBuffereven when the validation settings are off. S3 and Azure send it asIf-Match. Local, HDFS, and Web compare the etag after the bytes are read. A mismatch throws.getOrSetDeletionVectordoes not insert when the loader throws, and the query fails instead of reading the current object under the old identity. An empty or weak etag is not a cache key.deserializeRoaringPositionBitmaprejects a bitmap whose cardinality would passPUFFIN_DV_MAX_MATERIALIZED_POSITIONSbefore expanding it, including when no cardinality was declared. A fully empty DAG is still the on-wire stand-in for a missingschema_transform. Any DAG with inputs or outputs is kept, and an old worker protocol rejects it.CacheBase::clear,setMaxCount, andsetMaxSizeInBytesare virtual, andPuffinFilesCacheoverrides them so the footer memo follows the same limits.ObjectInfoInArchiveandObjectStorageQueueObjectInfoimplementclone.Tests
CreateReadBufferCanPinListedEtagWhenValidationIsOffchecks thatIf-Matchis sent only when the pin is requested.PuffinDeletionVectorCardinality.RejectsOmittedCardinalityAboveMaterializationLimitbuilds a run-length bitmap of 100000001 positions and expects a rejection before expansion.PuffinFooterMemo.BasePointerUpdatesFooterMemoclears and shrinks the footer memo through aCacheBasereference.ClusterFunctionReadTaskResponse.RoundTripsOutputOnlySchemaTransformOnSupportedProtocolkeeps an added-column transform, and the old protocol rejects it.ObjectInfoClonechecks that an archive entry and a queue entry stay their own types afterclone.