Skip to content

[Feature] add binlog meta module (row type) (1/3)#62058

Merged
morningman merged 5 commits into
apache:masterfrom
Userwhite:aime/1774246786-binlog-schema-pick
May 6, 2026
Merged

[Feature] add binlog meta module (row type) (1/3)#62058
morningman merged 5 commits into
apache:masterfrom
Userwhite:aime/1774246786-binlog-schema-pick

Conversation

@Userwhite
Copy link
Copy Markdown
Contributor

@Userwhite Userwhite commented Apr 2, 2026

What problem does this PR solve?

Issue: #61956

Row Binlog Schema

 the columns from base schema must be the same. Binlog system columns are invisible by default.
 +-----+-------+-------------------+--------------------+-------------------+--------------------------+
 | key | value | __BEFORE__value__ |__DORIS_BINLOG_LSN__|__DORIS_BINLOG_OP__|__DORIS_BINLOG_TIMESTAMP__|
 +-----+-------+-------------------+--------------------+-------------------+--------------------------+

row binlog table is actually a duplicate table model within origin table.

FE Meta

  1. We reuse MaterializedIndexMeta to restore binlog meta (including schema, col_unique_id)

  2. RowBinlogTableWrapper(OlapTableWrapper) is provided for read row binlog

  3. RowBinlog only support LightSchemaChange now

image

BE Meta

  1. RowBinlog is actually a rowset with TabletMeta, it has own data directory.

  2. introduce binlog_delvec for partial-update parellel

  3. We use GroupRowsetBuilder to construct data rowset and row binlog rowset.

image

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

  • Behavior changed:

    • No.
    • Yes.

  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@Thearas
Copy link
Copy Markdown
Contributor

Thearas commented Apr 2, 2026

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented Apr 2, 2026

run buildall

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 2, 2026

FE UT Coverage Report

Increment line coverage 53.25% (303/569) 🎉
Increment coverage report
Complete coverage report

@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented Apr 2, 2026

run buildall

@doris-robot
Copy link
Copy Markdown

doris-robot commented Apr 2, 2026

Cloud UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 78.48% (1798/2291)
Line Coverage 64.16% (32278/50310)
Region Coverage 65.00% (16175/24886)
Branch Coverage 55.45% (8618/15542)

@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented Apr 3, 2026

run buildall

@morningman morningman self-assigned this Apr 3, 2026
@dataroaring dataroaring requested a review from Copilot April 3, 2026 03:13
@dataroaring
Copy link
Copy Markdown
Contributor

dataroaring commented Apr 3, 2026

/review

github-actions[bot]
github-actions Bot reviewed Apr 3, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Found 2 correctness issues.

  1. be/src/storage/rowset_builder.cpp: GroupRowsetBuilder::commit_txn() only attaches the row-binlog rowset to local cleanup state, but TxnManager still persists/publishes a single rowset. In the normal publish path only the data rowset is made visible, so the companion row-binlog rowset is never added to tablet metadata and is also not recoverable across restart.
  2. be/src/storage/tablet/tablet.cpp: Tablet::create_initial_rowset() builds the initial row-binlog rowset with tablet_schema() instead of row_binlog_tablet_schema(), so the version-1 row-binlog rowset meta carries the base schema rather than the row-binlog schema.

Checkpoint Conclusions

  • Goal / completion / proof: The goal is to add row-binlog schema/meta support. The FE schema/meta pieces are largely in place, but the BE implementation is not end-to-end correct because the paired row-binlog rowset does not participate in the normal txn publish lifecycle, and there is no test covering that path. The added UTs cover builder/schema shape only, not publish/bootstrap behavior.
  • Small / focused change: The patch is feature-scoped but broad across FE, BE, thrift and proto. The main remaining problems are integration gaps rather than code organization.
  • Concurrency: The modified paths do involve concurrent txn/publish flows. I did not find a new lock-order regression in the inspected code, but the rowset lifecycle across txn/publish is incomplete.
  • Lifecycle management: Yes, special lifecycle management was added for the companion row-binlog rowset. This checkpoint fails: the extra rowset is tracked for local cleanup, but not for durable txn persistence/publish.
  • Configuration items: New binlog format / historical-value options were added and FE/BE serialization appears wired through. I did not verify dynamic-update semantics beyond that.
  • Compatibility: Proto/thrift/meta formats were extended. I did not prove a mixed-version break from the inspected code, but I also did not find explicit rolling-upgrade handling for the new feature.
  • Parallel code paths: Yes. The initial-rowset/bootstrap path is a functionally parallel path to normal write/publish and currently uses the wrong schema for row-binlog rowsets.
  • Special conditional checks: The new checks mostly follow existing style; no separate blocking issue here.
  • Test coverage: Insufficient for a storage feature of this risk level. Missing coverage for commit/publish/recovery and initial-rowset creation with row binlog enabled.
  • Observability: Existing logs are probably adequate for debugging the inspected paths; no blocking observability issue found.
  • Transaction / persistence correctness: This checkpoint fails. The extra row-binlog rowset is not carried through the persisted txn state and publish flow.
  • Data write / modification correctness: This checkpoint fails. The feature intends paired data+row-binlog writes, but only the data rowset becomes visible in the normal BE publish flow.
  • FE/BE variable passing: The create-tablet path appears updated to pass row-binlog schema/config. I did not find a concrete missing FE->BE field propagation bug in the reviewed code.
  • Performance: No primary performance blocker identified; correctness issues dominate.
  • Other issues: The bootstrap row-binlog schema bug above is an additional independent issue.

Overall opinion: not ready to merge until the BE txn/publish lifecycle handles the row-binlog companion rowset atomically and the row-binlog initial rowset is built with the correct schema.

Comment thread be/src/storage/rowset_builder.cpp
Comment thread be/src/storage/tablet/tablet.cpp
Copilot AI reviewed Apr 3, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces row-format binlog metadata and plumbing across FE/BE (schema/meta persistence, create-tablet RPC, rowset building, and reporting) to support the new binlog.format = ROW mode described in #61956.

Changes:

  • Extends RPC/metadata schemas (Thrift/Proto) to carry row binlog schema, row-binlog rowset metas, and binlog size/file count reporting.
  • Adds FE support for row binlog schema generation, row-binlog index meta management, and DDL restrictions / allowed alter ops for row-binlog tables.
  • Adds BE support for row binlog rowset lifecycle (separate path, rowset meta flag, group rowset builder/writer) plus delete-bitmap separation (binlog_delvec).

Reviewed changes

Copilot reviewed 100 out of 100 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
gensrc/thrift/MasterService.thrift Add binlog size/file count fields to tablet report payload.
gensrc/thrift/BackendService.thrift Add binlog size/file count to tablet stat report.
gensrc/thrift/AgentService.thrift Add binlog format enum, historical-value flag, and row binlog schema in create-tablet request.
gensrc/proto/olap_file.proto Add row-binlog flags/metas/schemas and delete-bitmap binlog marker; add binlog format + historical-value fields.
fe/fe-core/src/test/java/org/apache/doris/task/AgentTaskTest.java Add unit test coverage for create-tablet thrift including row binlog schema.
fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/TruncateTableCommandTest.java Add truncate test for row-binlog-enabled table.
fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ShowDataCommandTest.java Update/extend SHOW DATA validation tests for BinlogSize column.
fe/fe-core/src/test/java/org/apache/doris/common/proc/ProcServiceTest.java Add tests ensuring proc titles include binlog columns.
fe/fe-core/src/test/java/org/apache/doris/catalog/OlapTableRowBinlogSchemaTest.java New unit tests for row binlog schema generation (before/after/system cols).
fe/fe-core/src/test/java/org/apache/doris/catalog/CreateTableTest.java Add create-table tests for row binlog, temp tables, and unsupported key models.
fe/fe-core/src/test/java/org/apache/doris/binlog/MockBinlogConfigCache.java Update mock config creation and enable checks for CCR-vs-row modes.
fe/fe-core/src/test/java/org/apache/doris/binlog/BinlogTestUtils.java Split CCR vs ROW test config helpers.
fe/fe-core/src/test/java/org/apache/doris/alter/SchemaChangeHandlerTest.java Add tests for schema change behavior/limits with row binlog enabled.
fe/fe-core/src/main/java/org/apache/doris/task/CreateReplicaTask.java Add optional row binlog schema payload when creating base index replicas.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowDataCommand.java Add BinlogSize/LocalBinlogSize columns and aggregate reporting.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ReplaceTableOp.java Allow replace-table op on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ReplacePartitionOp.java Allow replace-partition op on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/RenameTableOp.java Allow rename-table op on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/RenameRollupOp.java Allow rename-rollup op on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/RenamePartitionOp.java Allow rename-partition op on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyTablePropertiesOp.java Add row-binlog allowlist logic; validate binlog.* properties via PropertyAnalyzer.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyTableCommentOp.java Allow comment changes on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyPartitionOp.java Allow partition-property changes on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyDistributionOp.java Allow distribution changes on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyColumnCommentOp.java Allow column comment changes on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/DropRollupOp.java Allow drop-rollup on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/DropPartitionOp.java Allow drop-partition on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/DropColumnOp.java Allow drop-column on row binlog tables (schema-change checks enforced elsewhere).
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AlterOp.java Introduce allowOpRowBinlog() default deny hook.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AddRollupOp.java Allow add-rollup on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AddPartitionOp.java Allow add-partition on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AddPartitionLikeOp.java Allow add-partition-like on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AddColumnsOp.java Allow multi-add-columns on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AddColumnOp.java Allow add-column on row binlog tables.
fe/fe-core/src/main/java/org/apache/doris/master/ReportHandler.java Pass row binlog meta when recovering/creating base-index replicas.
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java Validate ROW binlog constraints; create row binlog meta on table creation; pass row binlog meta into create-replica tasks.
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java Extend binlog property parsing for format + need_historical_value.
fe/fe-core/src/main/java/org/apache/doris/common/util/BufferSizeUtil.java Reserve extra id buffer (incl. binlog index id).
fe/fe-core/src/main/java/org/apache/doris/common/proc/TabletsProcDir.java Add proc columns for BinlogSize/BinlogFileNum.
fe/fe-core/src/main/java/org/apache/doris/common/proc/ReplicasProcNode.java Add proc columns for BinlogSize/BinlogFileNum.
fe/fe-core/src/main/java/org/apache/doris/catalog/TabletStatMgr.java Track table/binlog sizes and update replica binlog stats from BE reports.
fe/fe-core/src/main/java/org/apache/doris/catalog/Tablet.java Add tablet-level binlog size aggregation helper.
fe/fe-core/src/main/java/org/apache/doris/catalog/TableProperty.java Persist/load binlog format + need_historical_value properties.
fe/fe-core/src/main/java/org/apache/doris/catalog/RowBinlogTableWrapper.java New wrapper to represent/scan row-binlog as a dup-key table schema view.
fe/fe-core/src/main/java/org/apache/doris/catalog/Replica.java Add persisted replica binlog size and binlog file count fields.
fe/fe-core/src/main/java/org/apache/doris/catalog/Partition.java Add partition-level binlog size aggregation helper.
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTableWrapper.java New base wrapper class delegating locks/partition access to origin table.
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java Add row-binlog meta/index management, schema generation, and binlog size stats.
fe/fe-core/src/main/java/org/apache/doris/catalog/MaterializedIndexMeta.java Add row binlog index id marker and helpers.
fe/fe-core/src/main/java/org/apache/doris/catalog/MaterializedIndex.java Add index-level binlog size aggregation helper.
fe/fe-core/src/main/java/org/apache/doris/catalog/LocalTabletInvertedIndex.java Propagate binlog size/file count from tablet reports into replica.
fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java Replay binlog config updates with “force” merge semantics.
fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java Treat DB-level “enabled” binlog as CCR snapshot mode only.
fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java Add row-binlog system column constants and column builders.
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.java Update stats ctor call site for added binlog fields.
fe/fe-core/src/main/java/org/apache/doris/catalog/BinlogConfig.java Add binlog format + need_historical_value; split CCR vs ROW enabling; property merge validation.
fe/fe-core/src/main/java/org/apache/doris/binlog/TableBinlog.java Treat “enabled” for CCR binlog GC decisions.
fe/fe-core/src/main/java/org/apache/doris/binlog/DBBinlog.java Treat “enabled” for CCR binlog GC decisions.
fe/fe-core/src/main/java/org/apache/doris/binlog/BinlogUtils.java Add helper to wrap row-binlog table names.
fe/fe-core/src/main/java/org/apache/doris/binlog/BinlogManager.java Interpret DB-level enable as CCR snapshot mode.
fe/fe-core/src/main/java/org/apache/doris/binlog/BinlogConfigCache.java Make DB/table enable checks CCR-only.
fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java Disallow restore into row-binlog-enabled local table; pass row binlog meta to create tasks.
fe/fe-core/src/main/java/org/apache/doris/backup/BackupHandler.java Disallow backup for row-binlog-enabled tables.
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeJobV2.java Pass row binlog meta when creating shadow replicas.
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java Enforce light-schema-only rules; propagate add/drop changes into row-binlog schema; update binlog config via PropertyAnalyzer + merge rules.
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java Update create-replica task signature (rowBinlogMeta param).
fe/fe-core/src/main/java/org/apache/doris/alter/AlterOperations.java Add row-binlog allowlist enforcement for alter ops.
fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java Invoke row-binlog allowlist checks when needed.
be/test/olap/rowset/group_rowset_builder_test.cpp New test validating row-binlog rowset meta/schema hash behavior in GroupRowsetBuilder flow.
be/src/storage/txn/txn_manager.cpp Gate “add to binlog” by CCR-only enable.
be/src/storage/tablet/tablet.h Add row binlog paths/schema accessors and enable checks; add row-binlog rowset map.
be/src/storage/tablet/tablet.cpp Initialize row-binlog rowsets; add dual-rowset add_rowset; report binlog sizes; create initial rowsets using group writer when enabled.
be/src/storage/tablet/tablet_meta.h Add row-binlog schema/hash/rowset metas and binlog delete bitmap; expose binlog size/file count.
be/src/storage/tablet/tablet_meta.cpp Persist/load row-binlog schemas/rowsets; split delete bitmap into normal vs binlog; factor schema init helper.
be/src/storage/tablet/tablet_manager.cpp Include binlog size/file count in tablet stat report.
be/src/storage/tablet/base_tablet.h Add row-binlog version tracker and row-binlog rowset map.
be/src/storage/tablet/base_tablet.cpp Reconstruct version tracker for row-binlog graph when needed.
be/src/storage/snapshot/snapshot_manager.cpp Minor formatting/no-op change in meta conversion flow.
be/src/storage/rowset/vertical_beta_rowset_writer.h Add vertical row-binlog writer stub; remove final on base vertical writer template.
be/src/storage/rowset/rowset_writer_context.h Add binlog writer options in writer context.
be/src/storage/rowset/rowset_meta.h Add is_row_binlog flag helpers.
be/src/storage/rowset/rowset_fwd.h Add forward decls/typedefs for rowset writer/builder.
be/src/storage/rowset/rowset_factory.h Add factory method for empty GroupRowsetWriter.
be/src/storage/rowset/rowset_factory.cpp Create RowBinlogRowsetWriter and GroupRowsetWriter for binlog/group flows.
be/src/storage/rowset/pending_rowset_helper.h Allow pending guard to cover multiple rowset ids.
be/src/storage/rowset/pending_rowset_helper.cpp Implement multi-rowset-id pending guard.
be/src/storage/rowset/group_rowset_writer.h New GroupRowsetWriter that forwards flush/build to underlying writers.
be/src/storage/rowset/group_rowset_writer.cpp Implement group flush/build.
be/src/storage/rowset/beta_rowset_writer.h Add RowBinlogRowsetWriter type.
be/src/storage/rowset/beta_rowset_writer.cpp Mark rowsets as row-binlog based on context; propagate index_id; copy row-binlog flag in meta build.
be/src/storage/rowset_builder.h Introduce GroupRowsetBuilder + RowBinlogRowsetBuilder; add attach-rowset/pending-id hooks.
be/src/storage/rowset_builder.cpp Implement group builder flow and pending-rowset-id attachment; update init/profile behavior.
be/src/storage/data_dir.cpp Include row-binlog rowset ids when loading delete bitmaps; route bitmap to normal vs binlog delvec.
be/src/storage/binlog.h Add row binlog prefixes/suffix constants.
be/src/storage/binlog_config.h Add binlog format + historical-value fields and helpers.
be/src/storage/binlog_config.cpp Parse/serialize new binlog format + historical-value fields.
be/src/load/delta_writer/delta_writer.h Remove now-unnecessary RowsetBuilder downcast helper.
be/src/load/delta_writer/delta_writer.cpp Use BaseRowsetBuilder::commit_txn() polymorphically.
be/src/load/delta_writer/delta_writer_context.h Add WriteRequestType and group-write request structure.
be/src/load/channel/tablets_channel.cpp Minor include/format adjustments (optional include).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread be/src/storage/rowset_builder.cpp
Comment thread be/src/storage/tablet/tablet.cpp Outdated
Comment thread fe/fe-core/src/main/java/org/apache/doris/common/proc/TabletsProcDir.java
Comment thread fe/fe-core/src/main/java/org/apache/doris/catalog/TabletStatMgr.java
Comment thread fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTableWrapper.java
Comment thread fe/fe-core/src/main/java/org/apache/doris/catalog/Replica.java
Comment thread fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowDataCommand.java
Comment thread be/src/storage/tablet/tablet.h
@dataroaring
Copy link
Copy Markdown
Contributor

dataroaring commented Apr 3, 2026

Code Review: [Feature] add binlog meta module (row type)

+3025 / -443 across 95 files

This PR introduces row-level binlog support for OlapTable — a new BinlogFormat.ROW mode that stores row-level change data as a separate materialized index within the origin table. Below are the issues found during review.

Critical Issues

1. TabletStatMgr.updateTabletStat() accidentally removes replica stat updates

The diff shows that the following lines were removed from updateTabletStat():

replica.setLocalInvertedIndexSize(stat.getLocalIndexSize());
replica.setLocalSegmentSize(stat.getLocalSegmentSize());
replica.setRemoteInvertedIndexSize(stat.getRemoteIndexSize());
replica.setRemoteSegmentSize(stat.getRemoteSegmentSize());
replica.setLastReportVersion(stat.isSetVisibleVersion() ? stat.getVisibleVersion() : Long.MAX_VALUE);

Only setBinlogSize() / setBinlogFileNum() should have been added. The removal of setLastReportVersion() is particularly dangerous — it affects replica health checks and clone decisions. This looks like an accidental deletion during merge/rebase.

2. enable_row_binlog() missing config::enable_feature_binlog check (BE)

// tablet.h
bool enable_row_binlog() const {
    return _tablet_meta->binlog_config().is_enable() &&
           _tablet_meta->binlog_config().isRowBinlogFormat();
}

Compare with enable_binlog() and enable_ccr_binlog() which both check config::enable_feature_binlog first. Row binlog would be active even when the global feature flag is disabled. Should be:

bool enable_row_binlog() const {
    return config::enable_feature_binlog &&
           _tablet_meta->binlog_config().is_enable() &&
           _tablet_meta->binlog_config().isRowBinlogFormat();
}

3. VerticalRowBinlogRowsetWriter returns NotSupported for all column operations (BE)

The factory creates VerticalRowBinlogRowsetWriter when is_vertical && is_binlog_writer(), but add_columns(), flush_columns(), and final_flush() all return NotSupported. This means loads will fail on row-binlog-enabled tables when vertical writing is triggered.

Major Issues

4. BinlogConfig.binlogFormat null for legacy deserialized configs (FE)

binlogFormat is a new @SerializedName field. Legacy serialized configs will have binlogFormat = null. While isEnableForCCR() handles null correctly (null != ROW → true), toProperties() does:

properties.put(PROPERTIES_BINLOG_FORMAT, String.valueOf(binlogFormat));

String.valueOf(null) → "null", which will cause BinlogFormat.valueOf("null") to throw IllegalArgumentException if these properties are parsed back.

5. Maps.filterValues() returns live view — thread safety risk (FE)

OlapTable.getIndexIdToMeta() and getIndexNameToId() now return Maps.filterValues() live views. Concurrent modifications to indexIdToMeta while iterating (e.g., in rebuildFullSchema()) can cause ConcurrentModificationException.

6. set_data_writer() dynamic_pointer_cast failure silently nulls the writer (BE)

void GroupRowsetWriter::set_data_writer(const RowsetWriterSharedPtr& txn_rowset_writer) {
    _txn_rowset_writer = std::dynamic_pointer_cast<BaseBetaRowsetWriter>(txn_rowset_writer);
}

If the cast fails, _txn_rowset_writer becomes null silently. All delegation methods (num_rows(), flush(), etc.) then crash with null deref. Needs a DCHECK(_txn_rowset_writer != nullptr) after the cast.

7. operator<< for std::vector<RowsetId> may not exist (BE)

In pending_rowset_helper.cpp, the CHECK macro logs << _rowset_ids which is a std::vector<RowsetId>. Unless Doris has a project-wide operator<< for vectors, this is a build break.

8. InternalCatalog semantic mismatch: getEnable() vs isEnableForCCR() (FE)

if (dbBinlogConfig.getEnable() && !createTableBinlogConfig.isEnableForCCR() && ...)

If the DB has binlog.format=ROW with enable=true, getEnable() returns true but isEnableForCCR() for the table returns false (also ROW format). This incorrectly blocks creating ROW-format tables under ROW-format databases.

Minor Issues

Issue Severity Location
Boolean.parseBoolean() never throws — need_historical_value validation unreachable Low PropertyAnalyzer.java
GroupRowsetWriter methods use LOG(FATAL) instead of returning Status::NotSupported — crashes BE on accidental invocation Medium group_rowset_writer.h
PendingRowsetGuard::drop() resets to 1-element vector with default RowsetId instead of empty Low pending_rowset_helper.cpp
binlog_format() getter returns int32_t for a BinlogFormatPB field (implicit enum-to-int narrowing) Low binlog_config.h
_need_before field is set but never read (dead code) Low rowset_writer_context.h
Resource leak window: if _txn_rs_builder->init() fails after _row_binlog_rowset_builder->init() succeeds, binlog rowset files may be orphaned Medium rowset_builder.cpp

Design Concerns

  • OlapTableWrapper extends OlapTable: This creates a fragile split-state where the wrapper's own indexIdToMeta, fullSchema, etc. can drift from the origin table. Any new method added to OlapTable that accesses internal state won't automatically delegate. An interface-based or composition pattern would be cleaner.

  • Schema evolution is partially manual: ADD/DROP COLUMN are handled but MODIFY COLUMN is explicitly blocked. Future schema change operations added to Doris could silently drift the row binlog schema if they're not also blocked/handled.

Test Coverage

  • No regression tests (checkbox unchecked in PR)
  • No tests for OlapTableWrapper / RowBinlogTableWrapper
  • No test for BinlogConfig serialization round-trip with new fields (especially null binlogFormat legacy compat)
  • No test for backup/restore rejection when row binlog is enabled
  • FE UT increment coverage: 53.25% (303/569 lines)

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 3, 2026

Cloud UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 78.48% (1798/2291)
Line Coverage 64.16% (32279/50310)
Region Coverage 65.03% (16184/24886)
Branch Coverage 55.48% (8622/15542)

@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented Apr 3, 2026

  1. commit_txn

it will be introduced by write module PR(not ready).

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 3, 2026

FE UT Coverage Report

Increment line coverage 52.65% (308/585) 🎉
Increment coverage report
Complete coverage report

@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented Apr 3, 2026

run buildall

1 similar comment
@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented Apr 3, 2026

run buildall

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 3, 2026

Cloud UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 78.48% (1798/2291)
Line Coverage 64.16% (32280/50310)
Region Coverage 65.04% (16186/24886)
Branch Coverage 55.55% (8633/15542)

@Userwhite Userwhite force-pushed the aime/1774246786-binlog-schema-pick branch from f1a7fce to 574d4bf Compare April 3, 2026 08:38
@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented Apr 3, 2026

run buildall

@doris-robot
Copy link
Copy Markdown

doris-robot commented Apr 3, 2026

Cloud UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 78.48% (1798/2291)
Line Coverage 64.13% (32262/50310)
Region Coverage 65.02% (16181/24886)
Branch Coverage 55.42% (8613/15542)

@doris-robot
Copy link
Copy Markdown

doris-robot commented Apr 3, 2026

TPC-H: Total hot run time: 29085 ms 
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 574d4bfe23f15e5b4c385a1da152390eb4a79228, data reload: false

------ Round 1 ----------------------------------
orders	Doris	NULL	NULL	0	0	0	NULL	0	NULL	NULL	2023-12-26 18:27:23	2023-12-26 18:42:55	NULL	utf-8	NULL	NULL	
============================================
q1	17658	3737	3716	3716
q2	q3	10676	882	603	603
q4	4688	473	364	364
q5	7456	1355	1139	1139
q6	203	163	136	136
q7	939	976	784	784
q8	9556	1417	1344	1344
q9	6213	5354	5299	5299
q10	6307	2041	1778	1778
q11	487	278	287	278
q12	699	404	279	279
q13	18148	2768	2159	2159
q14	287	293	265	265
q15	q16	906	861	787	787
q17	1106	1126	868	868
q18	6517	5691	5514	5514
q19	1378	1183	1080	1080
q20	557	423	289	289
q21	4842	2492	2035	2035
q22	478	423	368	368
Total cold run time: 99101 ms
Total hot run time: 29085 ms

----- Round 2, with runtime_filter_mode=off -----
orders	Doris	NULL	NULL	150000000	42	6422171781	NULL	22778155	NULL	NULL	2023-12-26 18:27:23	2023-12-26 18:42:55	NULL	utf-8	NULL	NULL	
============================================
q1	4584	4351	4373	4351
q2	q3	4585	4701	4132	4132
q4	2033	2137	1379	1379
q5	4894	4933	5130	4933
q6	192	169	137	137
q7	1980	1786	1638	1638
q8	3324	3176	3267	3176
q9	8236	8366	8196	8196
q10	4457	4423	4232	4232
q11	783	419	377	377
q12	652	700	489	489
q13	2619	3064	2457	2457
q14	300	320	278	278
q15	q16	742	806	699	699
q17	1289	1265	1241	1241
q18	7881	6937	6944	6937
q19	1128	1122	1133	1122
q20	2230	2207	1921	1921
q21	5886	5215	4725	4725
q22	581	525	472	472
Total cold run time: 58376 ms
Total hot run time: 52892 ms

@doris-robot
Copy link
Copy Markdown

doris-robot commented Apr 3, 2026

TPC-DS: Total hot run time: 178468 ms 
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 574d4bfe23f15e5b4c385a1da152390eb4a79228, data reload: false

query5	4358	666	522	522
query6	337	225	201	201
query7	4224	597	333	333
query8	335	241	217	217
query9	8716	3884	3868	3868
query10	444	335	307	307
query11	6652	5483	5152	5152
query12	190	130	133	130
query13	1281	625	462	462
query14	5656	5180	4760	4760
query14_1	4169	4124	4099	4099
query15	221	204	184	184
query16	1063	461	448	448
query17	1172	764	618	618
query18	2621	477	371	371
query19	227	194	158	158
query20	140	135	129	129
query21	231	144	118	118
query22	14665	15035	14413	14413
query23	18112	17037	16587	16587
query23_1	16912	16678	16846	16678
query24	7653	1719	1336	1336
query24_1	1340	1361	1352	1352
query25	611	486	439	439
query26	1257	323	189	189
query27	2662	640	368	368
query28	4453	1881	1894	1881
query29	960	673	559	559
query30	301	240	204	204
query31	1098	1052	928	928
query32	81	70	69	69
query33	514	360	295	295
query34	1196	1182	673	673
query35	742	770	670	670
query36	1286	1224	1065	1065
query37	154	102	84	84
query38	3098	3000	2967	2967
query39	915	887	868	868
query39_1	833	838	843	838
query40	236	148	144	144
query41	62	60	59	59
query42	110	104	106	104
query43	310	315	303	303
query44	
query45	197	195	194	194
query46	1132	1237	785	785
query47	2341	2325	2233	2233
query48	397	421	297	297
query49	649	517	422	422
query50	753	297	213	213
query51	4364	4263	4263	4263
query52	109	109	95	95
query53	250	263	208	208
query54	320	272	256	256
query55	99	92	90	90
query56	312	326	305	305
query57	1713	1699	1697	1697
query58	299	269	273	269
query59	2879	2964	2768	2768
query60	334	325	318	318
query61	157	147	157	147
query62	680	624	574	574
query63	238	196	199	196
query64	5264	1313	978	978
query65	
query66	1465	465	376	376
query67	24328	24302	24295	24295
query68	
query69	450	362	320	320
query70	975	999	996	996
query71	314	290	266	266
query72	3168	2942	2644	2644
query73	828	787	449	449
query74	9867	9789	9631	9631
query75	2775	2634	2331	2331
query76	2312	1129	802	802
query77	404	402	333	333
query78	11238	11427	10713	10713
query79	1495	1052	765	765
query80	726	567	505	505
query81	468	280	239	239
query82	1298	158	120	120
query83	346	293	257	257
query84	262	146	121	121
query85	854	487	453	453
query86	390	346	309	309
query87	3296	3208	3084	3084
query88	3552	2712	2673	2673
query89	436	386	353	353
query90	1995	168	177	168
query91	182	165	139	139
query92	78	76	70	70
query93	962	971	565	565
query94	550	350	288	288
query95	641	453	329	329
query96	995	763	320	320
query97	2678	2691	2571	2571
query98	237	226	230	226
query99	1084	1078	968	968
Total cold run time: 257858 ms
Total hot run time: 178468 ms

@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented Apr 5, 2026

run buildall

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 28, 2026

PR approved by at least one committer and no changes requested.

@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented Apr 28, 2026

run buildall

@github-actions github-actions Bot removed the approved Indicates a PR has been approved by one committer. label Apr 28, 2026
gavinchou
gavinchou previously approved these changes Apr 29, 2026
View reviewed changes
@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Apr 29, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 29, 2026

PR approved by at least one committer and no changes requested.

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 29, 2026

run buildall

@gavinchou
Copy link
Copy Markdown
Contributor

gavinchou commented Apr 30, 2026

/review

github-actions[bot]
github-actions Bot previously requested changes Apr 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

I found blocking correctness issues in the current row-binlog metadata plumbing. Critical checkpoint conclusions: the PR goal is to add row-binlog metadata/schema plumbing, but production BE tablet creation can fail for row-binlog tables because the initial row-binlog rowset writes to a directory that is never created; tests do not prove the production path because they create that directory manually. The change is broad across FE/BE metadata and not yet fully integrated with all lifecycle paths. Concurrency/locking changes reviewed here do not introduce a confirmed new lock-order issue. Lifecycle/persistence is affected: row-binlog hidden index names are created but table rename is allowed without updating them. No new config item issue found beyond already-known review threads. FE/BE protocol enum compatibility for the remaining formats appears aligned. Testing exists, but it misses the production create-table directory path and rename lifecycle case. Observability is not the main concern for these failures. User focus: no additional user-provided review focus was supplied.

Comment thread be/src/storage/tablet/tablet.cpp
Comment thread fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/RenameTableOp.java
Comment thread fe/fe-core/src/main/java/org/apache/doris/catalog/Replica.java
hello-stephen
hello-stephen previously approved these changes Apr 30, 2026
View reviewed changes
CalvinKirs
CalvinKirs previously approved these changes Apr 30, 2026
View reviewed changes
@Userwhite Userwhite dismissed stale reviews from CalvinKirs, hello-stephen, and gavinchou via e39265c April 30, 2026 06:17
@github-actions github-actions Bot removed the approved Indicates a PR has been approved by one committer. label Apr 30, 2026
@Userwhite Userwhite force-pushed the aime/1774246786-binlog-schema-pick branch from e39265c to c286a46 Compare April 30, 2026 06:20
@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented Apr 30, 2026

run buildall

2 similar comments
@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 30, 2026

run buildall

@Userwhite
Copy link
Copy Markdown
Contributor Author

Userwhite commented May 2, 2026

run buildall

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label May 6, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 6, 2026

PR approved by at least one committer and no changes requested.

@morningman morningman merged commit 6d774eb into apache:master May 6, 2026
29 of 31 checks passed
@VividByteWorker
Copy link
Copy Markdown

VividByteWorker commented May 8, 2026

ref #57921

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@morningman morningman morningman approved these changes

@gavinchou gavinchou gavinchou approved these changes

@CalvinKirs CalvinKirs CalvinKirs left review comments

@hello-stephen hello-stephen hello-stephen left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@dataroaring dataroaring Awaiting requested review from dataroaring dataroaring is a code owner

@yiguolei yiguolei Awaiting requested review from yiguolei yiguolei is a code owner

+1 more reviewer

@yujun777 yujun777 yujun777 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@morningman morningman

Labels

approved Indicates a PR has been approved by one committer. binlog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.