Skip to content

Conversation

@Sachuman
Copy link
Contributor

@Sachuman Sachuman commented Sep 16, 2025

metamorphic: add lazy load SeekPrefixGE opt

This commit enables new lazy load optimization (maxsuffixprop) on metamorphic test
with probability of 20%

Part of #2002
Previous #5256

@Sachuman Sachuman requested a review from a team as a code owner September 16, 2025 21:18
@Sachuman Sachuman requested a review from xxmplus September 16, 2025 21:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@Sachuman Sachuman force-pushed the adding-seekprefixge-lazyload-to-metamorphic branch 2 times, most recently from a31ace0 to 7498302 Compare September 16, 2025 21:44
@Sachuman Sachuman changed the title cockroachkvs: add functions for MaximumSuffixProperty interface to cockroachkvs cockroachkvs: add functions for MaximumSuffixProperty interface Sep 17, 2025
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good!

Read up to the metamorphic test commit but not the last commit. I have some suggestions on the metamorphic test integration. If you want, you can pull the other commits out into separate PRs to get them merged earlier

@jbowens reviewed 1 of 1 files at r1, 9 of 9 files at r9, 9 of 9 files at r10, 2 of 7 files at r11.
Reviewable status: 3 of 10 files reviewed, 8 unresolved discussions (waiting on @xxmplus)


metamorphic/config.go line 445 at r11 (raw file):

	// MaximumSuffixProperty returns the maximum suffix property used during
	// the lazy position of SeekPrefixGE optimization.
	MaximumSuffixProperty() pebble.MaximumSuffixProperty

instead of putting this on the generator, I think this should be a field on the KeyFormat in metamorphic/config.go


cockroachkvs/blockproperties.go line 151 at r9 (raw file):

type MaxMVCCTimestampProperty struct{}

// Name is part of the cockroachkvs.MaxMVCCTimestampProperty interface.

nit: let's say "Name implements the pebble.MaximumSuffixProperty interface."


cockroachkvs/blockproperties.go line 156 at r9 (raw file):

}

// Extract is part of the cockroachkvs.MaxMVCCTimestampProperty interface.

s/Extract implements the pebble.MaximumSuffixProperty interface./


cockroachkvs/blockproperties.go line 170 at r9 (raw file):

		return nil, false, nil
	}
	// Decode the block interval using the same logic as sstable.decodeBlockInterval

nit: I think we should export sstable.DecodeBlockInterval and call it here since we're tightly coupled to the sstable's block interval property encoding


cockroachkvs/blockproperties.go line 193 at r9 (raw file):

	dst = append(dst, make([]byte, 9)...)
	binary.BigEndian.PutUint64(dst[len(dst)-9:], interval.Upper)
	dst[len(dst)-1] = 9

nit: instead of inlining 9, let's use suffixLenWithWall


sstable/reader_iter_two_lvl.go line 470 at r10 (raw file):

		// the top of the iterator's heap, and the iterator is Nexted.
		// During a RangeDelIter, the prefix passed and the prefix in the key might not be the same.
		if ok && maxSuffix != nil && i.secondLevel.reader.Comparer.ComparePointSuffixes(key[len(i.secondLevel.reader.Comparer.Split.Prefix(key)):], maxSuffix) < 0 {

ditto: regarding calling Split directly


sstable/reader_iter_single_lvl.go line 867 at r10 (raw file):

		// the top of the iterator's heap, and the iterator is Nexted.
		// During a RangeDelIter, the prefix passed and the prefix in the key might not be the same.
		if ok && maxSuffix != nil && i.reader.Comparer.ComparePointSuffixes(key[len(i.reader.Comparer.Split.Prefix(key)):], maxSuffix) < 0 {

nit: to extract the suffix index, we can just call Split directly like:key[i.reader.Comparer.Split(key):]


metamorphic/ops.go line 581 at r11 (raw file):

}

func deserializeMaximumSuffixProperty(s string) pebble.MaximumSuffixProperty {

rather than serializing the name of the implementation, I think we just serialize "maxsuffixprop" vs "none"; then when MaximumSuffixProperty we use can be determined by the KeyFormat used by the test

@Sachuman Sachuman force-pushed the adding-seekprefixge-lazyload-to-metamorphic branch 2 times, most recently from a03530b to 080bc35 Compare September 18, 2025 19:15
@Sachuman Sachuman marked this pull request as draft September 18, 2025 19:28
@Sachuman Sachuman changed the title cockroachkvs: add functions for MaximumSuffixProperty interface metamorphic: add lazy load SeekPrefixGE opt Sep 18, 2025
@Sachuman Sachuman force-pushed the adding-seekprefixge-lazyload-to-metamorphic branch from 080bc35 to b510f1c Compare September 18, 2025 20:27
This commit enables new lazy load optimization on metamorphic test
with probability of 20%

Part of cockroachdb#2002
Previous cockroachdb#5256
@Sachuman Sachuman force-pushed the adding-seekprefixge-lazyload-to-metamorphic branch from b510f1c to 5728d8a Compare September 18, 2025 20:31
Copy link
Contributor Author

@Sachuman Sachuman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Reviewable status: 0 of 14 files reviewed, 8 unresolved discussions (waiting on @jbowens and @xxmplus)


metamorphic/config.go line 445 at r11 (raw file):

Previously, jbowens (Jackson Owens) wrote…

instead of putting this on the generator, I think this should be a field on the KeyFormat in metamorphic/config.go

Done.


metamorphic/ops.go line 581 at r11 (raw file):

Previously, jbowens (Jackson Owens) wrote…

rather than serializing the name of the implementation, I think we just serialize "maxsuffixprop" vs "none"; then when MaximumSuffixProperty we use can be determined by the KeyFormat used by the test

Done.

@Sachuman Sachuman marked this pull request as ready for review September 18, 2025 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants