Skip to content

sstable: add code generator for sstable properties #5099

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Jul 21, 2025

The code to load/encode properties uses a mixture of relfection and
unsafe code and is fairly hard to understand. This commit switches to
using generated code instead. The generated code is straightforward to
understand.

Fixes #5098.

@RaduBerinde RaduBerinde requested a review from jbowens July 21, 2025 22:09
@RaduBerinde RaduBerinde requested a review from a team as a code owner July 21, 2025 22:09
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion (waiting on @jbowens)


sstable/properties.go line 443 at r1 (raw file):

		p.saveUvarint(m, unsafe.Offsetof(p.RawPointTombstoneValueSize), p.RawPointTombstoneValueSize)
	}
	if p.NumRangeKeys() > 0 {

This conditional block is the source of the test differences: we no longer include these fields when the value is zero.

@RaduBerinde
Copy link
Member Author

sstable/internal/genprops/gen_props.go line 1 at r1 (raw file):

// Copyright 2025 The LevelDB-Go and Pebble Authors. All rights reserved. Use

This was mostly written by o3 (https://chatgpt.com/share/e/687f9a51-6c38-8010-8c57-7682aecfea17). Feels strange to add this copyright to it, I'm not sure what the convention should be.

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 20 files reviewed, 2 unresolved discussions (waiting on @jbowens and @RaduBerinde)


sstable/internal/genprops/gen_props.go line 1 at r1 (raw file):

Previously, RaduBerinde wrote…

This was mostly written by o3 (https://chatgpt.com/share/e/687f9a51-6c38-8010-8c57-7682aecfea17). Feels strange to add this copyright to it, I'm not sure what the convention should be.

Let's leave this copyright header in place. Ack on the strangeness, but no copyright or some other copyright notice feels even stranger.

@RaduBerinde
Copy link
Member Author

RaduBerinde commented Jul 28, 2025

TODO: add a tag to allow interning of specific values like the merger name etc. Done

The code to load/encode properties uses a mixture of relfection and
unsafe code and is fairly hard to understand. This commit switches to
using generated code instead. The generated code is straightforward to
understand.
@RaduBerinde RaduBerinde force-pushed the sstable-properties-gen branch from aa743c7 to df16419 Compare July 28, 2025 18:14
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.

sstable: don't intern CompressionStats
3 participants