[python] BLOB descriptor parsing and read-path compatibility. - #9148
[python] BLOB descriptor parsing and read-path compatibility.#9148Stephen0421 wants to merge 1 commit into
Conversation
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Inline comments focus on behavioral regressions and consistency with the Java implementation.
| data = blob_value.to_data() | ||
| crc32 = self._write_with_crc(data, crc32) | ||
| else: | ||
| expected_length = self._expected_blob_length(blob_value) |
There was a problem hiding this comment.
Please restrict the exact-length path to an exact BlobRef (for example, type(blob_value) is BlobRef). This currently trusts to_descriptor().length for every Blob subtype, so a custom subtype whose new_input_stream() exposes more bytes than its descriptor is silently truncated. I reproduced a descriptor length of 3 with a stream containing abcdef; this branch writes only abc, while the base branch writes all 6 bytes. The Java writer deliberately checks blob.getClass() == BlobRef.class and reads other implementations to EOF.
There was a problem hiding this comment.
Thanks for catching this. Agreed — the exact-length copy path should be restricted to plain BlobRef, matching Java's blob.getClass() == BlobRef.class check.
Updated to use type(blob_value) is BlobRef before calling _copy_exactly. Other Blob subtypes (e.g. resolved BlobView) now read to EOF as before. Removed the test that expected truncation on resolved BlobView, since that behavior was incorrect.
| value = self.options.get(CoreOptions.BLOB_DESCRIPTOR_FIELD, None) | ||
| if isinstance(value, str): | ||
| value = value.strip() | ||
| if not value: |
There was a problem hiding this comment.
Please choose the legacy fallback based on key presence rather than value truthiness. Java Options.applyWithOption consults fallback keys only when the canonical key is absent. With blob-descriptor-field="" and blob.stored-descriptor-fields set, Java resolves an empty descriptor-field set, but this code revives the legacy fields. That can make Python interpret ordinary BLOB bytes as descriptors or choose a different write layout. An explicitly present blank canonical value should win.
There was a problem hiding this comment.
Good point. The legacy fallback should only apply when the canonical key is absent, not when it is explicitly set to a blank value.
Updated blob_descriptor_fields() to consult blob.stored-descriptor-fields only when blob-descriptor-field is None, matching Java Options.applyWithOption / parseCommaSeparatedSet semantics. With blob-descriptor-field="" and legacy set, Python now resolves an empty descriptor-field set, same as Java.
| return None | ||
| uri_length = struct.unpack('<I', raw[offset:offset + 4])[0] | ||
| total = offset + 4 + uri_length + 16 | ||
| if total != len(raw): |
There was a problem hiding this comment.
from_descriptor_bytes is used when the schema/storage context already says that the value is a descriptor, but this exact-length check rejects v1 bytes with trailing padding. Java BlobDescriptor.deserialize accepts trailing bytes for every supported version, so a padded legacy v1 descriptor can be read by Java but is rejected by Python (and the new test currently codifies that mismatch). Please deserialize v1 with the same Java semantics here, or coordinate a strict contract change on both implementations.
There was a problem hiding this comment.
Agreed — from_descriptor_bytes should follow Java BlobDescriptor.deserialize semantics and accept trailing padding for both v1 and v2.
Switched to always calling BlobDescriptor.deserialize() in from_descriptor_bytes, and updated the test to verify padded v1 descriptors are accepted. The heuristic from_bytes entry point still uses v2 magic only (is_blob_descriptor) so inline v1-shaped payload bytes are not misclassified.
Also aligned from_bytes(allow_blob_data=False) with Java Blob.fromBytes: when allow_blob_data=False, any bytes are deserialized as a descriptor (including v1), not only v2 magic-prefixed ones.
f20aa56 to
0c0865d
Compare
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
One remaining Java-compatibility issue in the legacy v1 BLOB view read path.
|
|
||
| if not CoreOptions.blob_as_descriptor(table.options): | ||
| return set() | ||
| return descriptor_field_indices( |
There was a problem hiding this comment.
[P2] When blob-as-descriptor=true, BlobInlineConvertReader resolves configured blob-view-field values to descriptor.serialize() before rows are exposed. If the upstream value is a legacy v1 descriptor, Python reserializes it as v1 without the magic header (unlike Java, whose BlobDescriptor.serialize() always writes CURRENT_VERSION). Because this helper marks only blob-descriptor-field, OffsetRow.get_blob() takes the heuristic path for the resolved view column and returns BlobData containing the descriptor bytes instead of a reference to the payload. Please either mark resolved view fields as descriptor-backed here or normalize reserialized descriptors to v2, and add a v1 blob-view regression test.
There was a problem hiding this comment.
Thanks for catching this. Fixed in the latest push:
- When
blob-as-descriptor=true, resolvedblob-view-fieldcolumns are now included in descriptor routing, soOffsetRow.get_blob()returnsBlobRefinstead ofBlobData. BlobDescriptor.serialize()now always writes v2 + magic (aligned with Java), so v1 descriptors re-serialized byBlobInlineConvertReaderare unambiguous.
Added test_offset_row_get_blob_v1_resolved_blob_view_field plus routing/serialize coverage.
Introduce explicit descriptor-byte parsing for managed BLOB v1/v2 reads, legacy blob.stored-descriptor-fields fallback, write-path truncation checks, UriReaderFactory lifecycle handling, and row-level descriptor field routing for blob-as-descriptor tables.
0c0865d to
a7975e5
Compare
2 participants
Summary
First PR in the stacked series for #9099. Shared descriptor-byte parsing and read/write foundations for managed BLOB v1/v2 compatibility — no PK-specific logic.
blob.py):from_bytes(v2 magic heuristic) vsfrom_descriptor_bytes(v1 strict + v2 deserialize with optional trailing padding)BlobInlineConvertReaderusesfrom_descriptor_bytesfor descriptor fieldsdescriptor_field_indiceswhenblob-as-descriptor=true→OffsetRow.get_blob()usesfrom_descriptor_bytesblob.stored-descriptor-fields; blankblob-descriptor-fieldtreated as unsetblob_format_writerrejects truncated copies (EOFError) when descriptor length is knownUriReaderFactoryowned FileIO tracking;clear_cache()without LRU double-close; FileIOclose()wiringBehavior changes (intentional)
ValueError(was silentBlobDatapassthrough)blob-descriptor-field=""+ legacy set → now falls back toblob.stored-descriptor-fieldsfrom_byteson arbitrary inline payload → unchanged (BlobData; v2-only heuristic)Test plan
BlobTestUriReaderFactoryTestFollow-ups
Related: #9099