Conversation
Torch3333
changed the title
Torch3333
changed the title
Torch3333
changed the title
dcdcd5e to
7c3b34a
Compare
…dering, equality, and key identity
82206d3 to
cf6b7be
Compare
There was a problem hiding this comment.
⚪ Unable to assess
Pull request overview
Aligns Consensus Commit’s in-memory key identity, filtering, validation, and scan overlap behavior with the configured collation.
Changes:
- Adds collation-aware snapshot keys, comparisons, and conditional validation.
- Propagates comparators through transaction managers and participants.
- Rejects unsupported pattern matching under ICU and expands unit coverage.
File summaries
| File | Description |
|---|---|
integration-test/.../ConsensusCommitSpecificIntegrationTestBase.java |
Passes configured comparator to CRUD handling. |
integration-test/.../ConsensusCommitNullMetadataIntegrationTestBase.java |
Updates comparator wiring. |
integration-test/.../ConsensusCommitImportTableIntegrationTestBase.java |
Updates comparator wiring. |
core/src/test/.../util/ScalarDbUtilsTest.java |
Tests collation and null-range filtering. |
core/src/test/.../WriteSetEncoderTest.java |
Updates snapshot-key construction. |
core/src/test/.../WriteSetDecoderTest.java |
Updates snapshot-key construction. |
core/src/test/.../TwoPhaseConsensusCommitManagerTest.java |
Configures default collation. |
core/src/test/.../TransactionContextTest.java |
Tests comparator-aware validation keys. |
core/src/test/.../SnapshotTest.java |
Extensively tests collation-aware snapshots. |
core/src/test/.../SnapshotKeyTest.java |
Tests canonical key identity. |
core/src/test/.../ParticipantCommitHandlerTest.java |
Updates comparator-aware fixtures. |
core/src/test/.../MutationsGrouperTest.java |
Tests byte-exact grouping behavior. |
core/src/test/.../MutationConditionsValidatorTest.java |
Tests collation-aware conditions. |
core/src/test/.../MergedResultTest.java |
Tests stored key spelling. |
core/src/test/.../CrudHandlerTest.java |
Tests collation-aware CRUD behavior. |
core/src/test/.../CoordinatorCommitHandlerWithGroupCommitTest.java |
Updates snapshot construction. |
core/src/test/.../ConsensusCommitParticipantTest.java |
Updates participant comparator wiring. |
core/src/test/.../ConsensusCommitOperationCheckerTest.java |
Tests ICU restrictions. |
core/src/test/.../ConsensusCommitManagerTest.java |
Tests manager-level ICU rejection. |
core/src/test/.../SelectStatementHandlerTest.java |
Reuses binary test comparator. |
core/src/test/.../ObjectStoragePartitionTest.java |
Reuses binary test comparator. |
core/src/test/.../MutateStatementHandlerTest.java |
Reuses binary test comparator. |
core/src/test/.../io/CollationComparators.java |
Adds shared test comparators. |
core/src/test/.../FilterableScannerTest.java |
Updates scanner comparator fixtures. |
core/src/test/.../checker/OperationCheckerTest.java |
Tests invalid conditional LIKE. |
core/src/main/.../util/ScalarDbUtils.java |
Removes fallback overload and handles null ranges. |
core/src/main/.../TwoPhaseConsensusCommitManager.java |
Propagates configured comparator. |
core/src/main/.../TransactionContext.java |
Builds validation keys with snapshot collation. |
core/src/main/.../Snapshot.java |
Implements collation-aware identity and validation. |
core/src/main/.../MutationConditionsValidator.java |
Applies collation to mutation conditions. |
core/src/main/.../MergedResult.java |
Preserves stored key spelling in columns. |
core/src/main/.../CrudHandler.java |
Applies collation throughout CRUD processing. |
core/src/main/.../ConsensusCommitParticipant.java |
Propagates comparator to participant snapshots. |
core/src/main/.../ConsensusCommitOperationChecker.java |
Enforces ICU operation restrictions. |
core/src/main/.../ConsensusCommitManager.java |
Propagates comparator to transactions. |
core/src/main/.../ConditionChecker.java |
Rejects pattern-matching mutation conditions. |
Review details
- Files reviewed: 36/36 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Byte-equal keys are collate-equal under every collation, so the common case of a re-read | ||
| // row skips collation-key generation. | ||
| if (namespace.equals(another.namespace) | ||
| && table.equals(another.table) | ||
| && partitionKey.equals(another.partitionKey) | ||
| && clusteringKey.equals(another.clusteringKey)) { | ||
| return true; |
| // No storage with a record or partition atomicity supports ICU collation | ||
| // If we support such storage in the future, we need to make the MutationsGrouper collation | ||
| // aware |
| if (!result.isPresent()) { | ||
| // A Put cannot rewrite a stored key, so a stored record keeps its own key spelling even when | ||
| // the Put's key only collates equal to it. | ||
| put.getPartitionKey().getColumns().forEach(c -> putColumns.put(c.getName(), c)); | ||
| put.getClusteringKey() |
| if (expression.getOperator() == Operator.LIKE | ||
| || expression.getOperator() == Operator.NOT_LIKE) { | ||
| // No storage evaluates pattern matching in a conditional write. | ||
| isValid = false; | ||
| break; | ||
| } |
There was a problem hiding this comment.
The LIKE/NOT_LIKE rejection added here doesn't take effect in Consensus Commit. ConsensusCommitOperationChecker calls ConditionChecker.check() for Put and Delete but discards the result:
Since ConditionChecker has no side effects, these calls currently do nothing. A LIKE condition that bypasses the builder check (e.g., via ConditionBuilder.putIf(List) or deleteIf(List)) passes the checker and, when the target record exists, hits default: throw new AssertionError() in MutationConditionsValidator.shouldMutate(). Other conditions that the storage-side OperationChecker rejects, such as a nonexistent column or a type mismatch, aren't rejected up front in Consensus Commit either.
This bug isn't introduced by this PR. The result has been discarded since the call was added in #899, and the same code is on 3 and 3.16 through 3.19. Could you fix it in a separate PR against master? The fix should be backported to all the supported release branches, which is easier as a standalone change than as part of this collation stack.
Some notes for that PR:
- Throwing
IllegalArgumentExceptionwhencheck()returnsfalse, asOperationChecker.checkCondition()does withCoreError.OPERATION_CHECK_ERROR_CONDITION, would align Consensus Commit with the storage API and JDBC transactions. - This is a behavior change. Conditions on primary-key columns and comparisons against a null value (e.g.,
isEqualToText(null)) are currently accepted and evaluated in memory; they would start to be rejected. ConsensusCommitOperationCheckerTestuses an unstubbedConditionCheckermock, which returnsfalse, so the existing tests that assert no exception would needcheck()stubbed to returntrue.
| // Arrange | ||
| Key partitionKey = Key.of(PKEY1, 1, PKEY2, "val1"); | ||
| Key clusteringKey = Key.of(CKEY1, 2, CKEY2, "val1"); | ||
| MutationCondition condition = new PutIf(ConditionBuilder.column(PKEY2).isLikeText("val%")); |
There was a problem hiding this comment.
This test passes without the new LIKE branch in ConditionChecker. PKEY2 is a partition key column, and ColumnChecker is constructed with requireNotPrimaryKey = true, so it already returns false for this column and OperationChecker throws IllegalArgumentException regardless of the operator. The test table has no non-key TEXT column, so one would be needed for this test to exercise the LIKE branch.
| this.collation = collation; | ||
| } | ||
|
|
||
| private void throwIfLikeConditionUnderIcuCollation(Selection selection) { |
There was a problem hiding this comment.
nit: The other private helpers in this class, including throwIfKeyedMutationAtomicityUnitUnderIcuCollation added in this PR, are placed after the public check methods. Could you move this one there as well?
| if (operator == Operator.LIKE || operator == Operator.NOT_LIKE) { | ||
| throw new IllegalArgumentException( | ||
| CoreError.COLLATION_ICU_LIKE_CONDITION_NOT_SUPPORTED.buildMessage( | ||
| operator, selection.forFullTableName().get(), condition.getColumn().getName())); |
There was a problem hiding this comment.
Question: Could you explain why LIKE/NOT_LIKE can't be supported under ICU?
| // No storage with a record or partition atomicity supports ICU collation | ||
| // If we support such storage in the future, we need to make the MutationsGrouper collation | ||
| // aware | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
This branch is effectively dead code: every storage whose mutation atomicity unit is RECORD or PARTITION rejects the ICU collation when it is created, and multi-storage doesn't allow overriding the collation per storage. How about throwing AssertionError instead and writing down why it's unreachable?
| // No storage with a record or partition atomicity supports ICU collation | |
| // If we support such storage in the future, we need to make the MutationsGrouper collation | |
| // aware | |
| throw new UnsupportedOperationException( | |
| // Unreachable: every storage whose mutation atomicity unit is RECORD or PARTITION rejects | |
| // the ICU collation when it is created (CoreError.COLLATION_ICU_NOT_SUPPORTED_BY_STORAGE), | |
| // and multi-storage doesn't allow overriding the collation per storage. If such a storage | |
| // supports the ICU collation in the future, revisit MutationsGrouper, which compares keys | |
| // byte-exactly. | |
| throw new AssertionError( |
checkForMutation_WithKeyedMutationAtomicityUnitUnderIcuCollation_ShouldThrowUnsupportedOperationException would need to be updated as well.
| .partitionKey(Key.ofText(ANY_NAME_1, ANY_TEXT_1.toLowerCase())) | ||
| .clusteringKey(Key.ofText(ANY_NAME_2, ANY_TEXT_2.toLowerCase())) |
There was a problem hiding this comment.
nit: ANY_TEXT_1 and ANY_TEXT_2 are already lowercase ("text1" and "text2"), so toLowerCase() doesn't change the spelling and this test passes without the change in MergedResult. Using a spelling that differs from the stored one (e.g., toUpperCase()) would make the test cover the change.
Description
Consensus Commit repeats part of the storage's work in memory: keying its read, write, and delete sets by record key, checking whether a buffered write overlaps a later scan, re-applying conditions to merged results, and validating conditional mutations. The previous PR in this stack made the storage-side in-memory comparisons follow
scalar.db.collation; this PR does the same here.Under ICU, collate-equal spellings name the same row, so the snapshot treats them as one key, mutation grouping puts them in one batch, and equality and range checks use the collation. Under the default BINARY collation, key identity stays byte-exact; the one default-path change is that TEXT range ordering in the scan-after-write check and in conditional-mutation validation moves from Java String order to unsigned UTF-8 byte order. Pattern matching here stays byte-exact at any collation and would disagree with an ICU backend, so transactions reject LIKE and NOT LIKE under ICU.
Related issues and/or PRs
This PR is the third of a stack of four PRs based on
feature/collationthat together replace #3791. It is based on the second PR's branch,collation-pr2-utf8-text-comparison.Stack, bottom to top: #3842, #3843, #3844, #3845.
Changes made
Snapshot.Keyidentity follows the collation: under ICU, TEXT key columns are identified by their collation key (CollationComparator.canonicalTextFormOf), so collate-equal keys hit one entry in the read, write, and delete sets and compare as equal; under BINARY, equality, hashing, and ordering are unchanged. The fields andtoStringkeep the original bytes.Snapshotcompares partition keys and tests clustering-key range membership with the collation's key comparator; inclusive boundaries are an ordering test, so a written key that collates equal to a boundary is in range. Conjunction re-evaluation on merged results and the scan-with-index column match use the collation's equality.MutationConditionsValidatordecides EQ and NE with the collation's equality and GT, GTE, LT, LTE with the collation's column comparator; IS_NULL and IS_NOT_NULL are unchanged.MutationsGroupergroups mutations by the same collation-aware key identity as the snapshot, so under ICU collate-equal partition or clustering keys land in one batch.CrudHandlerkeeps a row returned by a Get with index whose stored index value collates equal to the queried value instead of filtering it out byte-exactly.ConsensusCommitOperationCheckerrejects a Get or Scan carrying a LIKE or NOT LIKE condition withCoreError.COLLATION_ICU_LIKE_CONDITION_NOT_SUPPORTEDwhenscalar.db.collationis ICU, at every isolation level.ConsensusCommitManager,TwoPhaseConsensusCommitManager, andConsensusCommitParticipantbuild oneCollationComparatorfromDatabaseConfigand pass it toSnapshot,CrudHandler,MutationsGrouper, and the operation checker; the constructors of those classes, ofSnapshot.Key, and ofMutationConditionsValidatortake it as a parameter.ScalarDbUtils.columnsMatchAnyOfConjunctionsoverload; its remaining callers,SnapshotandCrudHandler, pass their own comparator.SnapshotKeyTestandMutationsGrouperTestcover key identity and grouping under BINARY and case-insensitive ICU (case variants, composite keys, null text, keys built with different comparators);SnapshotTestcovers read-your-own-write, write-set merging, scan-after-write validation, and serializable validation with collate-equal keys, range boundaries, index values, and conjunctions under both collations;MutationConditionsValidatorTestcovers EQ, NE, and GT on text and non-text under both collations;CrudHandlerTestcovers the Get-with-index match;ConsensusCommitOperationCheckerTestandConsensusCommitManagerTestcover the LIKE and NOT LIKE rejection for Get, Scan, and ScanAll.consensuscommitunit tests and the threeConsensusCommit*IntegrationTestBaseclasses in theintegration-testmodule to the new constructor arities.Checklist
Additional notes (optional)
This PR adds no integration tests; the
integration-testmodule changes only follow the new constructor arities. The next PR in this stack carries the integration tests and CI.Release notes
N/A