feat: let WriteManifest set and return v3 first_row_id#1321
feat: let WriteManifest set and return v3 first_row_id#1321the-onewho-knocks wants to merge 2 commits into
Conversation
Adds WithManifestFileFirstRowID, a ManifestFileOption that sets first_row_id on a v3+ data manifest. WriteManifest now accepts variadic ManifestFileOption args and forwards them to ToManifestFile, so callers can set and observe first_row_id through the public WriteManifest helper without affecting existing callers. Fixes apache#1274 Signed-off-by: the-onewho-knocks <borsehardik@gmail.com>
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
LGTM. The variadic opts plumbing through WriteManifest is additive and backward-compatible, and the v3 round-trip test (write with the option, read back, assert inheritance) is exactly right. A few small nits inline around v3-only semantics and coverage; none blocking.
| // WithManifestFileFirstRowID sets the first_row_id on a v3+ data manifest. | ||
| func WithManifestFileFirstRowID(firstRowID int64) ManifestFileOption { | ||
| return func(mf *manifestFile) { | ||
| mf.FirstRowIDValue = &firstRowID |
There was a problem hiding this comment.
nit: mf.version is already set before options are applied, so this could self-gate to v3+ where first_row_id actually exists:
if mf.version >= 3 {
mf.FirstRowIDValue = &firstRowID
}As written, calling this against a v1/v2 writer returns a ManifestFile whose FirstRowID() is non-nil but gets silently dropped on serialize. WithManifestFileContent has the same no-guard pattern so I'm fine either way, but the v3 exclusivity is a bit stronger here.
There was a problem hiding this comment.
added the mf.version >= 3 guard in b6f3bbb, with a doc comment noting it's a no-op on v1/v2 agreed it's worth being explicit here even though WithManifestFileContent doesn't guard the same way.
| m.EqualValues(1000+liveCount, *read[2].DataFile().FirstRowID()) | ||
| } | ||
|
|
||
| func (m *ManifestTestSuite) TestWriteManifestWithFirstRowIDOption() { |
There was a problem hiding this comment.
nit: consider adding (a) a v1/v2 + WithManifestFileFirstRowID case to pin the intended behavior (no-op vs non-nil FirstRowID()), and (b) a delete-manifest case, since the spec restricts first_row_id to data manifests. The read side already gates inheritance on formatVersion >= 3 && content == data, so this just locks down the write side.
There was a problem hiding this comment.
Added both in b6f3bbb:
a v1 manifest + WithManifestFileFirstRowID case confirming it's a no-op (FirstRowID() is nil)
a v3 delete-manifest case confirming the field can be set on the manifest itself (the guard only checks version), but entries don't inherit first_row_id on read since that's separately gated on content == data in the reader
| schema *Schema, | ||
| snapshotID int64, | ||
| entries []ManifestEntry, | ||
| opts ...ManifestFileOption, |
There was a problem hiding this comment.
nit (pre-existing): WriteManifest is exported but has no doc comment. Since you're already touching the signature, a one-liner noting that opts set v3-specific descriptor fields (e.g. WithManifestFileFirstRowID) would be a welcome add.
There was a problem hiding this comment.
Added a doc comment on WriteManifest in b6f3bbb, calling out that opts can include v3-specific options like WithManifestFileFirstRowID.
Fixes apache#1274 Signed-off-by: the-onewho-knocks <borsehardik@gmail.com>
2 participants
Summary
WithManifestFileFirstRowID, aManifestFileOptionthat setsfirst_row_idon a v3+ data manifestManifestFileOptionargs throughWriteManifesttoToManifestFileWhy
WriteManifestpredates the v3 work in #735 and forwarded no options toToManifestFile, soManifestFile.FirstRowID()was alwaysnilfor manifests written through this public helper, even thoughManifestWriter/ToManifestFilealready supported setting it viaopts. Library consumers had no way to set or observefirst_row_idthroughWriteManifest.WriteManifest's signature change is purely additive (optsis the last, variadic parameter), so existing call sites are unaffected.Testing
go test ./... -run '^TestWriteManifestWithFirstRowIDOption$' -count=1go test ./... -count=1gofmt -l .golangci-lint runFixes #1274