feat: Adds property based testing framework for compaction #26783

devanbenz · 2025-09-08T21:03:14Z

Adds property based testing framework for our compaction testings.
Current we support checking for "gaps" within adjacent compaction groups.

For example:

			expectedResult: func() TestLevelResults {
				return TestLevelResults{
					// Our rogue level 2 file should be picked up in the full compaction
					level4Groups: []tsm1.PlannedCompactionGroup{
						{
							tsm1.CompactionGroup{
								"000016844-000000002.tsm",
								"000016948-000000004.tsm",
								"000016948-000000005.tsm",
								"000017076-000000004.tsm",
								"000017094-000000004.tsm",
							},
							tsdb.DefaultMaxPointsPerBlock,
						},
					},
					// Other files should get picked up by optimize compaction
					level5Groups: []tsm1.PlannedCompactionGroup{
						{
							tsm1.CompactionGroup{
								"000016684-000000007.tsm",
								"000016684-000000008.tsm",
								"000016684-000000009.tsm",
								"000016684-000000010.tsm",
								"000016812-000000004.tsm",
								"000016812-000000005.tsm",
								"000017095-000000005.tsm",
							},
							tsdb.DefaultMaxPointsPerBlock,
						},
					},
				}
			},
		},

Would be invalid and cause our validator to fail due to the file "000017095-000000005.tsm" in level5Groups.

philjb · 2025-09-08T22:19:42Z

tsdb/engine/tsm1/compact_test.go

@@ -4517,6 +4498,10 @@ func TestEnginePlanCompactions(t *testing.T) {
 			e.Scheduler.SetDepth(1, mockGroupLen)
 			e.Scheduler.SetDepth(2, mockGroupLen)

+			// Validate our test case based on compact_property_test.go
+			validationErr := ValidateTestCase(test, test.expectedResult())


looks like this is validating the expectedResults() twice: once as they are extracted from test and then again as the second argument.

It should be validationErr := ValidateTestCase(test, level1Groups, level2Groups, Level3Groups, Level4Groups, Level5Groups)) where the groups come from e.PlanCompactions()... line 4526/4511

davidby-influx · 2025-09-08T23:53:02Z

This seems unnecessarily complex for what it does. Would something as simple and stupid as https://goplay.tools/snippet/sKHiARnmzi9 work instead?

philjb · 2025-09-09T05:34:33Z

tsdb/engine/tsm1/compact_property_test.go

+				betweenGroup, inGroup := fileToGroup[betweenFile.filename]
+
+				if inGroup && betweenGroup != groupIdx {
+					return fmt.Errorf("gap detected: files %s and %s are in group %d, but file %s (between them) is in group %d",


expected results: No Gaps Rule violation: gap detected: files 04-04.tsm and 09-04.tsm are in group 1, but file 05-03.tsm (between them) is in group 0

This error message is tough as the group indices don't tell me anything about which level group it came from. I think it should know which level group its talking about.

also: "no gaps rule" is odd description of the property. Maybe its me: the rule is that tsm files within a group must be adjacent in the master list of files. "Adjacency rule" seems more accurate. a "gap" sounds like a missing tsm file instead of one in the wrong place.

I also think the validation should tell me if the problem is in the expected results (aka I made a bad test case) or in the output of the compaction planner (the compaction planner is wrong). I know if the expected results are correct and valid and the compaction planner produces a bad plan, the test case will still fail, but more information is always helpful.

nvm -- i see that it does this!

also, it'd be nice if all instances of the issues were reported instead of stopping at the first adjacency rule violation.

philjb · 2025-09-09T05:36:55Z

tsdb/engine/tsm1/compact_test.go

+				level4Groups: Level4Groups,
+				level5Groups: Level5Groups,
+			})
+			require.NoError(t, validationErr, "test validation failed")


let the rest of the test run; assert don't require.

Sounds good 👍

philjb · 2025-09-09T06:08:31Z

tsdb/engine/tsm1/compact_property_test.go

+// validateNoGaps checks that there are no gaps within the same compaction level
+// A gap occurs when files A and C are in the same level group, but file B (between A and C)
+// is in a different level group, creating a "hole" in the sequence
+func validateNoGaps(allFiles []string, groups []tsm1.CompactionGroup) error {


the algorithm in here is strange; why did you write it this way? I believe it works but it seems more complicated than it needs to be.

It also sorts the files that are in the input CompactionGroups... which hides errors of files being out of order in the group (yes this should get caught elsewhere too).

I don't understand why the algorithm isn't:

parse the master file list filenames to get gens and seqs

sort by generation and then seq within generation into a "sorted master slice"

invert slice to a map[filename]->index in sorted master list

for each group, for each file in the group check if file[i] has an index in the master map that is -1 from file[i+1] in the master map.

This will fail if the group's files aren't sorted or if the group's files aren't adjacent.

e.g.
sorted master files list -> index in sorted slice

0-5 -> 0 1-5 -> 1 2-4 -> 2 3-4 -> 3 4-4 -> 4 5-2 -> 5 6-1 -> 6 7-1 -> 7

Any group has to be a strict subset.

Group0: <-- PASS
1-5 -> 1
2-4 -> 2

Group1:
3-4 -> 3
4-4 -> 4
6-1 -> 6 <--- failure. index 6 is not 4+1. Adjacency failure.

Group2:
6-1 -> 6
5-2 -> 5 <--- failure. index 5 is not 6+1; group file ordering failure.

For the Group1, it'd be easy to print the files in between because you have the index 4 and index 6, so you print the file at index 5 e.g.

Oh nice. I just noticed that @davidby-influx proposed exactly the same algorithm 6 hours ago in his comment.

devanbenz · 2025-09-09T14:40:43Z

While rewriting the algorithm for this I stumbled upon this test case (which I believe is wrong)

		{
			// This test will mock a 'backfill' condition where we have a single
			// shard with many generations. The initial generation should be fully
			// compacted, but we have some new generations that are not. We need to ensure
			// the optimize planner will pick these up and compact everything together.
			name: "Backfill mock condition",
			files: []tsm1.ExtFileStat{
				{
					FileStat: tsm1.FileStat{
						Path: "01-05.tsm1",
						Size: 2048 * 1024 * 1024,
					},
					FirstBlockCount: tsdb.DefaultAggressiveMaxPointsPerBlock,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "01-06.tsm1",
						Size: 2048 * 1024 * 1024,
					},
					FirstBlockCount: tsdb.DefaultAggressiveMaxPointsPerBlock,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "01-07.tsm1",
						Size: 2048 * 1024 * 1024,
					},
					FirstBlockCount: tsdb.DefaultAggressiveMaxPointsPerBlock,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "02-04.tsm1",
						Size: 700 * 1024 * 1024,
					},
					FirstBlockCount: tsdb.DefaultAggressiveMaxPointsPerBlock,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "02-05.tsm1",
						Size: 500 * 1024 * 1024,
					},
					FirstBlockCount: tsdb.DefaultAggressiveMaxPointsPerBlock,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "02-06.tsm1",
						Size: 400 * 1024 * 1024,
					},
					FirstBlockCount: tsdb.DefaultMaxPointsPerBlock,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "03-02.tsm1",
						Size: 700 * 1024 * 1024,
					},
					FirstBlockCount: tsdb.DefaultAggressiveMaxPointsPerBlock,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "03-03.tsm1",
						Size: 500 * 1024 * 1024,
					},
					FirstBlockCount: tsdb.DefaultAggressiveMaxPointsPerBlock,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "03-04.tsm1",
						Size: 400 * 1024 * 1024,
					},
					FirstBlockCount: tsdb.DefaultMaxPointsPerBlock,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "04-01.tsm1",
						Size: 700 * 1024 * 1024,
					},
					FirstBlockCount: tsdb.DefaultMaxPointsPerBlock,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "04-02.tsm1",
						Size: 500 * 1024 * 1024,
					},
					FirstBlockCount: 100,
				},
				{
					FileStat: tsm1.FileStat{
						Path: "03-03.tsm1",
						Size: 400 * 1024 * 1024,
					},
					FirstBlockCount: 10,
				},
			},
			testShardTime: -1,
			expectedResult: func() TestLevelResults {
				return TestLevelResults{
					level5Groups: []tsm1.PlannedCompactionGroup{
						{
							tsm1.CompactionGroup{
								"01-05.tsm1",
								"01-06.tsm1",
								"01-07.tsm1",
								"02-04.tsm1",
								"02-05.tsm1",
								"02-06.tsm1",
								"03-02.tsm1",
								"03-03.tsm1",
								"03-04.tsm1",
								"03-03.tsm1",
								"04-01.tsm1",
								"04-02.tsm1",
							}, tsdb.DefaultAggressiveMaxPointsPerBlock},
					},
				}
			},
		},

Notice the generation 3 TSM file at the end of the test input. This would be a violation. The property test validator should account for this as well I assume?

I'm removing complexity, getting rid of the sort for input that would cause this to pass, and cleaning this up.

philjb · 2025-09-09T15:37:32Z

Notice the generation 3 TSM file at the end of the test input. This would be a violation. The property test validator should account for this as well I assume?

The input tsm file list has a duplicate named tsm file, namely 03-03.tsm1, in the test you reference. So yes, that's a mistake in the test setup.

For this pr; please focus on just the adjacency aspect; let's get that right and merged so we can return to PF; we can add other checks (unique list in input files, sorted files in groups, etc) in separate prs -- they are checks are properties too, which is nice, but they are less important.

The most important thing is that we don't change the ordering of input so this will stop that, as we check the index anyways 🤔

philjb · 2025-09-09T17:32:18Z

tsdb/engine/tsm1/compact_property_test.go

+func validateFileAdjacency(allFiles []string, groups []tsm1.CompactionGroup) error {
+	var inputFiles []fileInfo
+	for i, file := range allFiles {
+		gen, seq, err := tsm1.DefaultParseFileName(file)


if you're not going to sort the input list, why do we need to parse the filename? We only need gen and seq for sorting the input file list.

Should we sort the input list? For tests, we know the files will be in order, but for random input from production code, does Glob have order guarantees?

philjb · 2025-09-09T17:34:16Z

tsdb/engine/tsm1/compact_property_test.go

+
+	var fileMap = make(map[string]fileInfo, len(inputFiles))
+	for _, file := range inputFiles {
+		fileMap[file.fileName] = file


without the sorting this can be just

for i, file := range allFiles { fileMap[file] = i }

Ah yeah, I can probably condense this down and just build the map directly now.

… by seq or gen

philjb

a couple minor changes but please make them.

philjb · 2025-09-10T18:34:10Z

tsdb/engine/tsm1/compact_property_test.go

+
+// validateFileAdjacency checks that there are no gaps between compaction groups
+// An adjacency violation occurs when files A and C are in different groups, but file B (between A and C)
+// is also in a different group, creating overlapping or non-contiguous ranges


This comment is confusing to me. It says if A and C are in different compaction groups and B is in a different group too that's a problem. I don't think that's a problem.

All the files within a planned compaction group must be adjacent with respect to the master list of files (aka allFiles).

Would you update this comment please?

How does this sound?

// validateFileAdjacency checks that there are no adjacency violations between TSM files // The following example will highlight an adjacency violation: // Given the following list of files [01-01.tsm, 02-02.tsm, 03-03.tsm] let's say we have the following compaction plans created // Group 1: [01-01.tsm, 03-03.tsm] & Group 2: [02-02.tsm] // This violates file adjacency as the first compaction group sees [01-01.tsm, X, 03-03.tsm] and the second group: [X, 02-02.tsm, X] // these are non-contiguous blocks, when the first group performs compaction we will have two files that are out of order compacted together.

sounds good.

You could also mention that we have this rule because the ordering of the files indicates which overwrite of a data point wins (newest write wins).

philjb · 2025-09-10T18:35:24Z

tsdb/engine/tsm1/compact_property_test.go

+	var errs []error
+
+	// Extract all filenames from test case
+	var allFiles []string


since you're going to touch this for the comment change, please preallocate this since you know how big it will be. ci pays for allocations too.

philjb · 2025-09-10T18:41:24Z

And there are some failing tests according to ci that need to be addressed.

davidby-influx

Some comments, perhaps actionable, but worth understanding the issues

davidby-influx · 2025-09-10T18:46:58Z

tsdb/engine/tsm1/compact_property_test.go

+	Validator:   validateFileAdjacency,
+}
+
+type fileInfo struct {


Is there a reason for the wrapping struct?

davidby-influx · 2025-09-10T18:49:18Z

tsdb/engine/tsm1/compact_property_test.go

+func validateFileAdjacency(allFiles []string, groups []tsm1.CompactionGroup) error {
+	var inputFiles []fileInfo
+	for i, file := range allFiles {
+		gen, seq, err := tsm1.DefaultParseFileName(file)


Should we sort the input list? For tests, we know the files will be in order, but for random input from production code, does Glob have order guarantees?

davidby-influx · 2025-09-10T18:51:48Z

tsdb/engine/tsm1/compact_property_test.go

+// An adjacency violation occurs when files A and C are in different groups, but file B (between A and C)
+// is also in a different group, creating overlapping or non-contiguous ranges
+func validateFileAdjacency(allFiles []string, groups []tsm1.CompactionGroup) error {
+	var fileMap = make(map[string]fileInfo, len(allFiles))


Should we sort files? Tests will surely have the correct order, but if we use this in Production code, does Glob have ordering promises?

Glob actually does not have ordering promises in production. When I was implementing a psuedo version of this in the PR where I implemented adjacency checks (https://github.com/influxdata/influxdb/pull/26766/files#diff-e0d6c334f09b26e18b81206ad3b242f2fd1ef09a7b23ea326a13eea3ac916a9aR2478-R2483) I did have to sort, if we were to ever pull this out and utilize it for other than tests I think that it would be beneficial to keep the sorting.

Tests will surely have the correct order

apparently not!

as well as a better description of the issue. Re-add sorting

philjb · 2025-09-10T19:48:12Z

tsdb/engine/tsm1/compact_test.go

@@ -3519,15 +3500,15 @@ func TestEnginePlanCompactions(t *testing.T) {
 				},
 				{
 					FileStat: tsm1.FileStat{
-						Path: "03-03.tsm1",


this is a duplicate tsm file in this test. there are two 03-03.tsm1 files in the input list.

The question is what do we think the intent of the test is. Do we

delete the duplicate

rename it as 05-03

rename it as 04-03

I think the intent was that this should be the third file in the 04 generation as the 04 generation is a level 1 generation. By making this file 05-03... there's now a level 3 generation after the level 1 generation. I don't think that's the intent of this test (although it is an interesting test case).

please rename to 04-03

philjb · 2025-09-10T19:51:22Z

tsdb/engine/tsm1/compact_test.go

-				return testLevelResults{
-					level1Groups: []tsm1.PlannedCompactionGroup{
-						{
-							tsm1.CompactionGroup{"05-01.tsm", "06-01.tsm", "07-01.tsm", "08-01.tsm", "09-01.tsm", "10-01.tsm", "11-01.tsm", "12-01.tsm"},


again, what is the intent of the original test. Mock another planned level inside scheduler aggress blocks middle is tough for me to understand, but you've significantly changed the expectations of this test so i think you've changed its intent too much. There's no level1 plan at all anymore!

I think you need to keep the tsm files the same names but properly sort them in files: []tsm1.ExtFileStat{ test setup. That should keep the test expectations unchanged. Please make it so

I agree. Sort the input test files, but change nothing else.

davidby-influx

As @philjb suggests

davidby-influx · 2025-09-10T20:01:14Z

tsdb/engine/tsm1/compact_test.go

-				return testLevelResults{
-					level1Groups: []tsm1.PlannedCompactionGroup{
-						{
-							tsm1.CompactionGroup{"05-01.tsm", "06-01.tsm", "07-01.tsm", "08-01.tsm", "09-01.tsm", "10-01.tsm", "11-01.tsm", "12-01.tsm"},


I agree. Sort the input test files, but change nothing else.

philjb

LGTM

davidby-influx

LGTM

devanbenz added 2 commits September 8, 2025 16:01

feat: Adds property based testing framework for compaction

2a73c97

feat: Adds common for compact tests

b708182

devanbenz marked this pull request as ready for review September 8, 2025 21:29

devanbenz requested review from davidby-influx, philjb and gwossum September 8, 2025 21:29

devanbenz self-assigned this Sep 8, 2025

philjb reviewed Sep 8, 2025

View reviewed changes

feat: Update with results

8d241d9

philjb reviewed Sep 9, 2025

View reviewed changes

devanbenz added 5 commits September 9, 2025 11:05

feat: Adjust AdjacentFileProperty to improve algorithm

c14a416

feat: Adds a log instead of requirement for violations in adjacency

a149a65

feat: I DO NOT think that I need to sort

f3498c0

The most important thing is that we don't change the ordering of input so this will stop that, as we check the index anyways 🤔

feat: Use assert for testing

5ace584

feat: rename method to validateFileAdjacency

061ade9

philjb reviewed Sep 9, 2025

View reviewed changes

devanbenz added 2 commits September 9, 2025 12:36

feat: Just build the map and use that

36e8106

feat: Removes parsing of TSM file name since we are no longer sorting…

0b7c018

… by seq or gen

philjb reviewed Sep 10, 2025

View reviewed changes

davidby-influx reviewed Sep 10, 2025

View reviewed changes

feat: Update comment to include information about why we want this rule

1320743

as well as a better description of the issue. Re-add sorting

philjb reviewed Sep 10, 2025

View reviewed changes

feat: Modify ordering in test to revert to original intent

817317e

davidby-influx requested changes Sep 10, 2025

View reviewed changes

devanbenz added 2 commits September 10, 2025 15:18

fix: make sure nothing except sort order is different in tests

98d0c6a

chore: rename test to make it more clear what is happening

912765e

philjb approved these changes Sep 10, 2025

View reviewed changes

davidby-influx approved these changes Sep 10, 2025

View reviewed changes

devanbenz merged commit 3f1f4a0 into master-1.x Sep 10, 2025
9 checks passed

devanbenz deleted the db/compaction-property-based-testing branch September 10, 2025 20:51

feat: Adds property based testing framework for compaction #26783

feat: Adds property based testing framework for compaction #26783

Uh oh!

Conversation

devanbenz commented Sep 8, 2025

Uh oh!

philjb Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidby-influx commented Sep 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philjb Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devanbenz commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philjb commented Sep 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philjb Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philjb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philjb commented Sep 10, 2025

Uh oh!

davidby-influx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philjb Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidby-influx left a comment

Choose a reason for hiding this comment

Uh oh!

philjb Sep 8, 2025 •

edited

Loading

philjb Sep 9, 2025 •

edited

Loading

devanbenz commented Sep 9, 2025 •

edited

Loading

philjb Sep 9, 2025 •

edited

Loading

philjb Sep 10, 2025 •

edited

Loading