[rust][client][server] Require only omitted columns to be nullable in partial update - #3885
Draft
gstamatakis95 wants to merge 1 commit into
Draft
[rust][client][server] Require only omitted columns to be nullable in partial update#3885gstamatakis95 wants to merge 1 commit into
gstamatakis95 wants to merge 1 commit into
Conversation
…al update Partial update required every non-primary-key column to be nullable, even columns explicitly listed in the target columns. A listed column is always supplied by the writer, so the requirement rejected valid usage. Restrict the requirement to omitted columns in the Rust client, the Java client and the server. Fixing only the clients is not enough, the server runs its own copy of the check. Auto increment columns keep the requirement. They are always omitted and only receive their value after the merge, so updateRow writes null into them first. Partial delete keeps the requirement on non-primary-key target columns, since it sets them to null. The check sits after the whole-row-removal short-circuit and only exists on the server, because legality depends on the stored row.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Partial update validation required every non-primary-key column of a primary key table to be nullable, including columns explicitly listed in the target columns. A listed column is supplied by the writer on every request, so the requirement rejected valid schemas. The loop's own comment described the intended behaviour ("check the columns not in targetColumns"), but the loop never consulted the target column set.
The check is duplicated across three layers that validate independently, and all three carried the defect:
fluss-rust/.../client/table/upsert.rs,UpsertWriterFactory::sanity_checkfluss-client/.../writer/UpsertWriterImpl.java,sanityCheckfluss-server/.../kv/partialupdate/PartialUpdater.java,sanityCheckThe reported symptom cannot be resolved in the Rust client alone. A client-only fix permits writer construction and defers the same rejection to the server at first write.
Resulting validation rules
updateRowwrites null into it when the row does not yet existNOT NULLdeleteRowsets it to null unless the whole row is removedDivergence from the fix proposed in the issue
The issue proposes retaining the auto-increment exemption (
!target_column_set[i] && !pk_column_set[i] && !auto_increment_column_set[i]). This PR removes it.Removing the exemption is the correct direction because of ordering on the write path.
PartialUpdater.updateRownull-fills omitted columns beforeAutoIncrementUpdaterassigns a value (KvTablet.processUpserttoapplyInserttoupdateAutoIncrementColumns). Relaxing the server instead would allow aNOT NULLauto increment column to reachBinaryWriter.createNotNullValueWriterholding null, producing an NPE surfaced to the client asUnknownServerException.Restored invariant
The previous rule made "a partial update row cannot carry null in a non-nullable slot" true by construction. Relaxing it removes that guarantee, and the decode path is not defensive:
InternalRow.createFieldGetterandCompactedRowReaderomit theisNullAtbranch for non-nullable types, and CompactedRow is a sequential variable-length encoding, so a null bit in a non-nullable slot desynchronises every subsequent field rather than yielding null. Two guards restore the invariant:updateRowrejects a null value supplied for aNOT NULLtarget column, primary key columns included. The check runs before any field getter, because the first getter deserialises the whole row with the non-null-checking readers and would fail first, with an error dependent on the bytes that follow the record in the batch.isNullAtreads only the null-bit header, so the check itself never deserialises.deleteRowrejects partial delete when a non-primary-key target column isNOT NULL. The guard is placed after theisFieldsNullshort circuit, so whole-row removal, which nulls nothing, remains legal.The two guards use separate column sets, since the delete guard must exempt the primary key while the update guard must not.
The delete guard is server-side only. Legality depends on the stored row, so a client-side equivalent would necessarily be stricter and would render a server-supported operation unreachable.
Simplification
The primary key bitset is removed from all three checks. The containment check that runs first proves every primary key column is a target column, so
!targetColumns.get(i)already excludes them. The bitset remains in the delete computation, which must exclude primary key columns. The auto increment bitset stays in all three checks, only to select the dedicated error message below.Error message
Changed from:
to:
A
NOT NULLauto increment column gets a dedicated message, because the user cannot follow the generic advice of listing the column, targeting an auto increment column is itself rejected:A repository-wide search confirmed no other code, test, golden file, documentation, or language binding referenced the previous wording.
Compatibility
No wire format, storage format, or public API signature changes. The change relaxes validation, so previously accepted writes remain accepted. No corruption is possible because no format changed, but the behaviour warrants a release note.
Known limitation
AggregationContext.sanityCheckTargetColumnsretains the stricter rule, becausePartialAggregateRowMergerreturns the new row unchanged on first write instead of null-filling omitted columns. Consequently, partial update on anaggregationmerge-engine table with aNOT NULLcolumn now fails on the server at first write rather than at writer creation. The failure is a cleanInvalidTargetColumnException. The behaviour is documented in the new docs section. Tracking the relaxation separately.Documentation
website/docs/table-design/table-types/pk-table.mdgains a nullability requirements section covering the relaxed rule for omitted columns, the stricter rule for partial delete, and the stricter rule kept by theaggregationmerge engine.Fixes #3849
Test plan
PartialUpdaterTest, 6 methods and 10 cases, constructor checks and both runtime guards parameterized over everyKvFormat, covering the relaxed rule, primary key containment, auto increment nullability, and a server-typed malformed row that proves the update guard fires before deserializationnot_null_column_in_target_columns_is_acceptedandomitted_not_null_auto_increment_column_is_rejected. The omitted-rejection case requested in the issue is already covered by the existingsanity_checktest, whose expected message is updated rather than duplicatedFlussTableITCase#testPartialPutWithNotNullTargetColumn, which assertsInvalidTargetColumnExceptionpropagates over RPC, andKvTabletTest#testPartialDeleteWithNotNullTargetColumnmvn test -Dtest='org.apache.fluss.server.kv.**' -pl fluss-serverpasses, 320 testsmvn spotless:check checkstyle:check validatepasses on affected modulescargo fmt --check,cargo clippy -D warningsandcargo deny check licensesclean!partialUpdateCols.get(i)failsPartialUpdaterTest. Dropping!target_column_set[i]failsnot_null_column_in_target_columns_is_accepted🤖 AI-assisted changes - reviewed by human developer