dumpformat: fix corrupted DECIMAL values in dictionary-encoded parquet files

Project: pingcap/tidb | Language: Go | Concepts: memory-management, data-structures, error-handling | Difficulty: mid
Link: https://github.com/pingcap/tidb/pull/70436

The problem

When importing Parquet files with dictionary-encoded DECIMAL columns, repeated values were sometimes corrupted. Only the first use of a dictionary entry would be correct; subsequent uses could decode to wrong values (like 0.00). This happened silently, and downstream checks would not catch it, because the corruption happened before values were written to storage.

Why it's non-trivial

The bug is caused by subtle buffer ownership and mutation issues:

• The Parquet decoding logic reused the same backing array for all rows referencing a dictionary entry.
• The decimal conversion function mutated its input buffer in place.
• As a result, after the first conversion, the shared buffer was changed, so all subsequent rows using the same dictionary entry would see incorrect data.

A naive fix (e.g., just changing the conversion logic or adding more checks) would not address the root cause: the need to isolate each conversion from shared, mutable buffers.

The approach

1. Add a safe decimal setter that copies the input before conversion

The core of the fix is a new helper that ensures the decimal conversion always works on a private copy of the bytes, not the shared dictionary buffer:

+// getDecimalByteSetter returns a setter for byte array DECIMAL columns.
+// Dictionary decoded values alias the shared dictionary buffer, and the
+// conversion consumes its input in place, so copy it into our own buffer first.
+func getDecimalByteSetter[T parquet.ByteArray | parquet.FixedLenByteArray](scale int) setter[T] {
+	var buf []byte
+	return func(val T, d *types.Datum) error {
+		buf = append(buf[:0], val...)
+		return setDatumFromDecimalByte(d, buf, scale)
+	}
+}

What this does:

• Defines a generic setter for both parquet.ByteArray and parquet.FixedLenByteArray types.
• Before converting, it copies the input bytes into a buffer owned by the setter, ensuring no mutation of shared memory.

2. Use the safe setter for all relevant DECIMAL paths

The old code passed the possibly-shared buffer directly to the conversion:

-		return func(val parquet.ByteArray, d *types.Datum) error {
-			return setDatumFromDecimalByte(d, val, int(converted.decimalMeta.Scale))
-		}

Now, it uses the new safe setter:

+		return getDecimalByteSetter[parquet.ByteArray](int(converted.decimalMeta.Scale))

And similarly for fixed-length byte arrays:

-		return func(val parquet.FixedLenByteArray, d *types.Datum) error {
-			return setDatumFromDecimalByte(d, val, int(converted.decimalMeta.Scale))
-		}
+		return getDecimalByteSetter[parquet.FixedLenByteArray](int(converted.decimalMeta.Scale))

What this does:

• Ensures that all code paths that decode decimal values from byte arrays (including both variable and fixed-length) use the safe copy-before-convert logic.

3. Add regression tests for repeated dictionary-encoded decimals

A new test ensures that repeated dictionary-encoded decimal values are correctly decoded, including both positive and negative values, fixed and variable length, and oversized decimals:

+	t.Run("dictionary_encoded_decimal", func(t *testing.T) {
+		// Enough repeated rows that the writer keeps the dictionary page instead of
+		// falling back to PLAIN, so every row decodes to the same shared buffer.
+		const rows = 256
+		...
+		expectedValues := []string{
+			"123.45", "-123.45", "123.45",
+			"4523128485832663883733241601901871400518358776001584532791311875309106626.56",
+		}
+		for range rows {
+			require.NoError(t, reader.ReadRow())
+			require.Len(t, reader.lastRow.Row, len(expectedValues))
+			for i, expectValue := range expectedValues {
+				s, err := reader.lastRow.Row[i].ToString()
+				require.NoError(t, err)
+				require.Equal(t, expectValue, s)
+			}
+		}
+	})

What this does:

• Writes and reads a Parquet file with 256 repeated values for each decimal column.
• Checks that every decoded value matches the expected string, ensuring the buffer fix works.

From the review thread

No notable review discussion on this PR.

Takeaways

• Never mutate shared buffers unless you own them: When working with data structures that may be shared (like dictionary entries in Parquet), always copy before mutating.
• Understand the memory semantics of third-party libraries: If a library returns slices or pointers, check if they are reused or aliased. Don't assume you have exclusive ownership.
• Regression tests should cover subtle memory bugs: Bugs involving shared state are often hard to spot; targeted tests with repeated values can catch these issues.