[core] Align dedicated Blob writes and URI resolution with Java - #989
Conversation
leaves12138
left a comment
There was a problem hiding this comment.
Reviewed 148b0d7, including the corresponding Java URI/error handling and row-kind filtering paths. The existing table, Blob-format, URI, dedicated/external-write, resource, row-limit, row-kind and Java-index-fixture suites passed locally (1,774 tests passed; 3 ignored). Two additional reproductions identified the issues below. The ignored-delete reproduction passes on parent fc27163 and fails on this head. Please address these before approval. The reproduction tests were run in an isolated checkout; no changes have been pushed to this PR.
|
|
||
| fn http_error(error: reqwest::Error) -> Error { | ||
| Error::UnexpectedError { | ||
| message: format!("HTTP Blob request failed: {error}"), |
There was a problem hiding this comment.
[P1] Redact signed HTTP URLs from both the error message and cause
reqwest::Error includes the request/response URL in its display output, and retaining it as source also exposes that URL when the error chain is formatted. I reproduced this through the public BlobReader::from_file_io(...).read_blobs(...) API: an unsigned /redirect descriptor redirects to /failure?signature=review-synthetic-secret, which returns HTTP 403. The returned Paimon error contains signature=review-synthetic-secret in both the message and the nested reqwest error. blob_error_with_context does not protect this case because it only replaces the original descriptor URI, not a different redirect target.
A signed URL is a credential, so ordinary HTTP failures would expose it in query/job logs. Please ensure HTTP errors and their causes never retain unredacted userinfo/query credentials, including redirected URLs, and sanitize the URI context added by the table read/write wrappers as well. Java's HttpClientUtils explicitly sanitizes HTTP failures rather than propagating the original exception. A redirect-to-signed-URL failure test should cover this path.
| return Ok(None); | ||
| } | ||
| if !self.primary_key_indices.is_empty() { | ||
| super::inline_blob::validate_inline_blob_columns( |
There was a problem hiding this comment.
[P2] Apply row-kind filtering before validating inline Blob payloads
The new validation runs before enrich_rowkind_batch, which is where filter_rowkind_batch discards ignored rows. With a primary-key table configured with blob-descriptor-field=payload and ignore-delete=true, a batch containing an INSERT with a valid descriptor and a DELETE with an empty payload now fails with BlobDescriptor bytes too short, instead of dropping the DELETE and writing the INSERT. I reproduced this with explicit _VALUE_KIND values [0, 3]; the same integration test passes on parent fc27163 and fails on this head. Ignored UPDATE_BEFORE rows are subject to the same ordering issue.
Java's TableWriteImpl.writeAndReturn returns for a filtered-out row before extracting or writing its Blob value. Please validate only the rows that survive row-kind generation/filtering, still before opening any physical files, and add a mixed retained/ignored-row regression test.
There was a problem hiding this comment.
Re-reviewed ae58da0. The two previously requested changes are addressed: HTTP failures no longer retain credential-bearing reqwest errors in the source chain, including redirect failures, and ignored row kinds are filtered before inline Blob validation. Also checked the descriptor-preserving partial-update change against the missing-external-object regression.
Local Rust verification: 1,767 tests passed, 3 ignored, across table unit tests, Blob format/URI unit tests, the HTTP-error, dedicated-Blob-write and external-data-write integration suites, and all 25 DataFusion Blob tests. Rebuilt the Python binding from this exact commit and ran it together with apache/paimon#10305: all 47 newly added native Blob write cases passed, including both native-commit option settings (DE tables retain the Python commit fallback) and Python/native cross-reads, rolling, streaming reuse, inline descriptors and HTTP streaming.
No remaining blocking issue found in this Rust change. The Python PR is reviewed separately: its explicit abort-after-prepare adapter contract and one Python-specific Ray fault-injection test still require follow-up, as recorded in the review there. Approval here does not mark that dependent PR's Native CI as passing.
Purpose
Companion PyPaimon integration: apache/paimon#10305 (draft until this core change is merged).
Close the core gaps exposed by enabling scalar BLOB Arrow writes in PyPaimon Native. This covers file grouping, rolling, metadata, descriptors and URI I/O as one write/read path.
Changes
0..row_count-1, honordata-evolution.write-cols-optimization.enabled, and generate indexes for the normal columns.The HTTP reader follows Java's decoded-stream offsets rather than HTTP wire ranges. Unknown-length descriptors may need an additional size pass; that pass does not retain the payload in memory.
Verification
Final local results:
cargo test --locked -p paimon --lib table::: 1,679 passed, 3 ignored.cargo test --locked -p paimon --lib arrow::format::blob::tests: 46 passed.cargo clippy --locked --all-targets --workspace --features fulltext,vortex -- -D warnings: passed.cargo fmt --all -- --checkandgit diff --check: passed.