[core] Align dedicated Blob writes and URI resolution with Java - #989
JingsongLi wants to merge 2 commits into
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.
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.