[core] Support BLOB fields in partial-update tables. - #8928
Conversation
JingsongLi
left a comment
There was a problem hiding this comment.
Why reject managed BLOB and BLOB view fields for partial-update tables?
|
JingsongLi
left a comment
There was a problem hiding this comment.
Requesting changes for two correctness issues in the newly enabled managed-BLOB partial-update path. The focused core tests and current CI pass, but neither case below is covered.
|
|
||
| checkArgument( | ||
| options.mergeEngine() == MergeEngine.DEDUPLICATE | ||
| || options.mergeEngine() == MergeEngine.PARTIAL_UPDATE, |
There was a problem hiding this comment.
Allowing PARTIAL_UPDATE here exposes an incompatible retract path for managed BLOBs. PrimaryKeyBlobExternalizer.externalize replaces every non-null managed BLOB with null for DELETE/UPDATE_BEFORE, while PartialUpdateMergeFunction passes that field to FieldAggregator.retract. With fields.ts.sequence-group=payload and fields.payload.aggregate-function=last_non_null_value, inserting (payload=A, ts=1) and then retracting (payload=A, ts=2) incorrectly keeps A, because a null retract field means ‘keep the accumulator.’ Please either preserve/externalize non-null managed BLOB values on retract (and retain their references), or reject retract-sensitive aggregation for managed BLOB fields. An end-to-end retract regression test would also be needed.
There was a problem hiding this comment.
Thanks for catching this. I fixed it at the schema-validation boundary.
For a managed BLOB field protected by a sequence group, retract-sensitive aggregate functions are now rejected because managed BLOB payloads are not retained in retract records. last_value remains supported because it can deterministically clear the field without the original payload, and other aggregate functions can be used with fields.<field>.ignore-retract=true.
The validation still allows safe cases such as ignore-delete=true and managed BLOB fields that are not protected by a sequence group.
I also added end-to-end coverage for newer and older retract sequences, compaction, empty compacted .blobref files, and ignore-retract with scalar, ARRAY, and MAP managed BLOB fields.
| return; | ||
| } | ||
|
|
||
| Set<String> managedBlobFields = |
There was a problem hiding this comment.
This validation only checks the global sequence.field; it misses sequence-group ordering fields encoded in option keys. For example, fields.payload.sequence-group=name is accepted when payload is a managed BLOB in a partial-update table. PartialUpdateMergeFunction.Factory later creates a comparator for payload, and code generation fails with IllegalArgumentException: Illegal type: BLOB. Please parse the key portion of every fields.<key[,key...]>.sequence-group option and reject managed BLOB, ARRAY<BLOB>, and MAP<K, BLOB> fields there, while continuing to allow BLOBs on the protected-value side.
There was a problem hiding this comment.
Thanks, fixed. Schema validation now parses the key portion of every partial-update fields.<key[,key...]>.sequence-group option and recursively rejects ordering fields containing BLOB, including ARRAY<BLOB> and MAP<K, BLOB>.
BLOB fields on the protected-value side remain supported. The parser is shared with PartialUpdateMergeFunction.Factory to keep schema validation and runtime behavior consistent.
I also added tests for scalar, ARRAY, MAP, multi-key ordering fields, allowed BLOB protected fields, malformed sequence-group options, and compatibility with non-partial-update merge engines.
5a5726f to
3f18792
Compare
Purpose
This PR adds BLOB support for primary-key tables using the
partial-updatemerge engine.The main changes include:
blob-descriptor-fieldwith partial updates.blob-fieldforBLOB,ARRAY<BLOB>, andMAP<K, BLOB>.blob-view-fieldresolution and unresolved-reference forwarding.Tests
SchemaValidationTestPartialUpdateMergeFunctionTestPrimaryKeyBlobFileWriterTestPrimaryKeyPartialUpdateBlobTestBlobViewTableReadSupportTestPrimaryKeySimpleTableTest#testReadWithLimitThroughTableReadPathMultiSplitPrimaryKeySimpleTableTest#testReadWithLimitThroughTableReadPathMultiSplitWithQueryAuthFilterPartialUpdateITCase#testBlobDescriptorPartialUpdatePartialUpdateITCase#testBlobDescriptorPartialUpdateSequenceGroupPartialUpdateITCase#testManagedBlobPartialUpdatePartialUpdateITCase#testManagedBlobPartialUpdateSequenceGroupPartialUpdateITCase#testBlobViewPartialUpdatePartialUpdateITCase#testBlobViewPartialUpdateForwardReferencePartialUpdateITCase#testBlobViewPartialUpdateSequenceGroup