Skip to content

feat(table): skip validator registration for no-op producers#1022

Merged
laskoviymishka merged 3 commits intoapache:mainfrom
hectar-glitches:main
May 7, 2026
Merged

feat(table): skip validator registration for no-op producers#1022
laskoviymishka merged 3 commits intoapache:mainfrom
hectar-glitches:main

Conversation

@hectar-glitches
Copy link
Copy Markdown
Contributor

Summary

This PR gates validator registration on a new needsValidation() bool method on the producerImpl interface:

  • fastAppendFiles returns false: appends are commutative and need no conflict checks
  • mergeAppendFiles inherits false via embedding
  • overwriteFiles returns true: real conflict validation required
  • snapshotProducer.commit() skips registration when needsValidation() is false

The no-op validate() methods are kept as belt-and-suspenders.

Behavior is locked with two unit tests asserting that fast-append and merge-append commits leave txn.validators empty.

Parent: #830

…e#830

Signed-off-by: hectar-glitches <hectar@uni.minerva.edu>
@hectar-glitches hectar-glitches requested a review from zeroshade as a code owner May 6, 2026 02:50
@laskoviymishka laskoviymishka changed the title table: skip validator registration for no-op producers. Parent: #830 feat(table): skip validator registration for no-op producers May 6, 2026
Copy link
Copy Markdown
Contributor

@laskoviymishka laskoviymishka left a comment

Choose a reason for hiding this comment

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

:shipit:
Thanks for contribution!

Skipping the no-op closure for fast/merge append matches Java’s BaseFastAppend / MergeAppend, which also don’t register validate() on the append path.

No correctness or spec concerns from my side.

A few nits before merging @hectar-glitches :

  • The comment above the gated if in commit() looks a bit stale now — “pay only the no-op-closure cost” no longer quite describes the new behavior.
  • A one-line doc comment on needsValidation() would make the interface contract clearer for future readers.
  • mergeAppendFiles.needsValidation is inherited through Go method promotion. That works, but an explicit override could make the intent easier to see locally.
  • If we keep relying on promotion, TestMergeAppendFiles_InheritsFastAppendValidator could also assert require.False(t, ma.needsValidation()).

Overall LGTM from my side.

// and merge-append).
validate(cc *conflictContext) error

needsValidation() bool
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worth a one-line doc on the contract here? The neighbour validate has a 7-line block (lines 50-56), and needsValidation is the gate that decides whether validate even runs — defining "return false iff validate is unconditionally a no-op" pins the invariant at the declaration. Without it, someone could add real work to a producer's validate() later without flipping the predicate and silently bypass the check.

return nil
}

func (fa *fastAppendFiles) needsValidation() bool { return false }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tiny robustness ask while we're here: I'd consider adding an explicit func (m *mergeAppendFiles) needsValidation() bool { return false } next to the mergeAppendFiles declaration so the choice is local to that type rather than inherited via Go method promotion from the embedded fastAppendFiles. Right now nothing at the mergeAppendFiles site signals the policy, and a future producer copy-pasting the embed pattern would silently inherit false with no compiler nudge. wdyt?

// only in unit tests that exercise commit() directly on a bare
// snapshotProducer; guard to keep those tests green.
if impl := sp.producerImpl; impl != nil {
if impl := sp.producerImpl; impl != nil && impl.needsValidation() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment block just above is now stale — it still says fast/merge-append pay only the no-op-closure cost, but with the new gate they pay nothing. I'd freshen the rationale while we're here, e.g. "Producers that are commutative against concurrent appends (fast-append, merge-append) opt out via needsValidation() == false and skip registration entirely. The nil guard remains for unit tests that drive commit() on a bare snapshotProducer."

@hectar-glitches
Copy link
Copy Markdown
Contributor Author

I'll get on those nit-picks

@laskoviymishka laskoviymishka merged commit 0c4b5b7 into apache:main May 7, 2026
14 checks passed
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.

2 participants