mzzz-zzm commented on code in PR #983: URL: https://github.com/apache/iceberg-go/pull/983#discussion_r3193720629
########## table/partition_conflict_test.go: ########## @@ -0,0 +1,108 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --------------------------------------------------------------------------- +// partitionTupleKey +// --------------------------------------------------------------------------- + +func TestPartitionTupleKey_Empty(t *testing.T) { + assert.Equal(t, "", partitionTupleKey(map[int]any{})) + assert.Equal(t, "", partitionTupleKey(nil)) +} + +func TestPartitionTupleKey_SingleField(t *testing.T) { + assert.Equal(t, "1=hello;", partitionTupleKey(map[int]any{1: "hello"})) +} + +func TestPartitionTupleKey_MultipleFieldsOrderedByID(t *testing.T) { + // Keys must be sorted by field ID, so insertion order must not matter. + p1 := map[int]any{1: "a", 3: "c", 2: "b"} + p2 := map[int]any{3: "c", 1: "a", 2: "b"} + assert.Equal(t, partitionTupleKey(p1), partitionTupleKey(p2), + "same content, different insertion order must produce identical keys") + assert.Equal(t, "1=a;2=b;3=c;", partitionTupleKey(p1)) +} + +func TestPartitionTupleKey_DifferentValuesProduceDifferentKeys(t *testing.T) { + pA := map[int]any{1: "us-east-1"} + pB := map[int]any{1: "eu-west-1"} + assert.NotEqual(t, partitionTupleKey(pA), partitionTupleKey(pB)) +} + +// --------------------------------------------------------------------------- +// validateNoConflictingDataFilesInPartitions — short-circuit paths +// --------------------------------------------------------------------------- + +// TestValidateNoConflictingDataFilesInPartitions_SnapshotIsolationIsNoOp +// verifies that the validator is a no-op under snapshot isolation, matching +// the behaviour of validateNoConflictingDataFiles. +func TestValidateNoConflictingDataFilesInPartitions_SnapshotIsolationIsNoOp(t *testing.T) { + head := int64(1) + meta := newConflictTestMetadata(t, &head) + ctx, err := newConflictContext(meta, meta, MainBranch, nil, true) + require.NoError(t, err) + + partitions := []map[int]any{{1: "us-east-1"}} + require.NoError(t, validateNoConflictingDataFilesInPartitions(ctx, partitions, IsolationSnapshot)) +} + +// TestValidateNoConflictingDataFilesInPartitions_EmptyInputs verifies that +// the validator short-circuits when there are no concurrent snapshots or no +// eq-delete partitions, without touching the filesystem. +func TestValidateNoConflictingDataFilesInPartitions_EmptyInputs(t *testing.T) { + head := int64(1) + meta := newConflictTestMetadata(t, &head) + ctx, err := newConflictContext(meta, meta, MainBranch, nil, true) + require.NoError(t, err) + + // No concurrent snapshots — must not read any manifests. + require.NoError(t, validateNoConflictingDataFilesInPartitions(ctx, []map[int]any{{1: "us-east-1"}}, IsolationSerializable)) + + // No eq-delete partitions — nothing to validate. + require.NoError(t, validateNoConflictingDataFilesInPartitions(ctx, nil, IsolationSerializable)) + require.NoError(t, validateNoConflictingDataFilesInPartitions(ctx, []map[int]any{}, IsolationSerializable)) +} + +// TestValidateNoConflictingDataFilesInPartitions_UnpartitionedFallsBack +// verifies that a single unpartitioned eq-delete (empty tuple) causes the +// validator to fall back to the conservative AlwaysTrue path. Because the +// conflictContext has no concurrent snapshots (fs = nil, concurrent = nil), +// the AlwaysTrue fallback itself returns nil — this test just ensures we +// reach and exercise that code path without panicking. +func TestValidateNoConflictingDataFilesInPartitions_UnpartitionedFallsBack(t *testing.T) { + head := int64(1) + // Build metadata with a concurrent snapshot by using two different base/current metas. + meta := newConflictTestMetadata(t, &head) + + // writerBase has no snapshot; current has head snapshot → head is concurrent. + ctx, err := newConflictContext(meta, meta, MainBranch, nil, true) + require.NoError(t, err) + + // Empty partition tuple → unpartitioned eq-delete → must fall back to AlwaysTrue. + // With no concurrent snapshots in ctx the AlwaysTrue path returns nil (no-op). + emptyPartition := map[int]any{} + require.NoError(t, validateNoConflictingDataFilesInPartitions(ctx, []map[int]any{emptyPartition}, IsolationSerializable)) +} Review Comment: All five requested cases are now present in `table/partition_conflict_test.go`: - `TestRowDeltaValidate_DifferentPartitionAllowed` — #978 reproducer: `eu-west-1` concurrent + `us-east-1` eq-delete → no conflict - `TestRowDeltaValidate_SamePartitionRejected` — `us-east-1` concurrent + `us-east-1` eq-delete → rejected - `TestRowDeltaValidate_UUIDPartitionSameRejected` / `TestRowDeltaValidate_UUIDPartitionDifferentAllowed` — UUID partition type safety (these would fail against `%v` keying) - `TestRowDeltaValidate_UnpartitionedTableFallsBackToAlwaysTrue` — replaces the trivial-pass test - `TestRowDeltaValidate_SpecEvolutionConflictDetected` — eq-delete written under spec A (`identity(region)`, partitionFieldID=1000), concurrent data written under spec B (renamed to `region_v2`, partitionFieldID=1001, same source field ID=2), both with value `"us-east-1"`. `eqDeletePartitionsToFilter` builds `Reference("region") == "us-east-1"` in row space; `validateAddedDataFilesMatchingFilter` projects it against spec B via `buildPartitionProjection` → conflict detected. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
