fix: validate changelog producer for first-row merge engine#342
Conversation
Signed-off-by: QuakeWang <wangfuzheng0814@foxmail.com>
JingsongLi
left a comment
There was a problem hiding this comment.
LGTM! Clean implementation with good test coverage.
A few minor observations:
-
The validation correctly covers both the
Schema::build()path (create-time) and theTableSchema::apply_changes()path (alter-time), as well as a runtime guard inTableWrite::new()for legacy schemas loaded from disk — nice defense in depth. -
The
first_row_supports_changelog_producerhelper ispub(crate)and shared betweencore_options.rsandtable_write.rs, keeping the logic in one place. -
Tests cover all the key scenarios: compatible producers (none, lookup), incompatible producers (input, full-compaction), alter-time rejection, and final-options validation when both merge-engine and changelog-producer change together.
One optional thought for a follow-up: if the MERGE_ENGINE_OPTION constant in schema.rs could be unified with the one already defined in core_options.rs, it would avoid having two identical string literals — but not a blocker for this PR.
Purpose
Prevent invalid
merge-engine=first-rowtables from using unsupportedchangelog-producervalues.first-rowonly supportsnoneandlookup.Brief change log
first-rowchangelog producer compatibility.Tests
API and Format
Documentation