zclllyybb commented on issue #65383:
URL: https://github.com/apache/doris/issues/65383#issuecomment-4914477690
Breakwater-GitHub-Analysis-Slot: slot_b528dc2e891b
This content is generated by AI for reference only.
Initial analysis for apache/doris#65383:
This looks like a real FE validation-order bug, not only a regression-test
expectation problem.
What I checked:
- The live issue is open, has no labels/comments at read time, and reports
Doris HEAD `d4a077caf8a` with a failing `ALTER TABLE tbl_row_fmt SET
("binlog.format" = "STATEMENT_AND_SNAPSHOT")`.
- The exact short commit is not visible from the public `apache/doris`
commit search I ran, but the same routing gap is present on refreshed
`upstream/master` (`16a745d5125`).
Code path:
- `ModifyTablePropertiesOp.validate()` recognizes `binlog.*` properties and
calls `PropertyAnalyzer.analyzeBinlogConfig(...)`, but this only validates the
property value shape. It does not compare the requested value with the existing
table binlog config.
- The comparison that would produce the expected message is in
`SchemaChangeHandler.updateBinlogConfig()`: it builds a new `BinlogConfig` and
calls `BinlogConfig.mergeFromProperties(..., false)`.
`BinlogConfig.mergeFromProperties()` explicitly returns `not support change
binlog format from ROW to STATEMENT_AND_SNAPSHOT` when `binlog.format` changes.
- However, `AlterOperations.checkBinlogConfigChange()` currently routes only
these keys to `updateBinlogConfig()`: `binlog.enable`, `binlog.ttl_seconds`,
`binlog.max_bytes`, and `binlog.max_history_nums`.
- It does not include `binlog.format`, so this statement falls through to
the generic `hasSchemaChangeOp()` path. In `SchemaChangeHandler.process()`,
`ModifyTablePropertiesOp` sets `lightSchemaChange = false`, and then the
row-binlog guard rejects it with `only support light schema change operator
when use table with binlog<Row>`.
So the wrong message is caused by routing `binlog.format` through the
schema-change path before the binlog-config merge/validation path can run.
Recommended next step:
- Extend the binlog-config routing predicate to include
`PropertyAnalyzer.PROPERTIES_BINLOG_FORMAT`.
- Also consider including
`PropertyAnalyzer.PROPERTIES_BINLOG_NEED_HISTORICAL_VALUE`, because
`BinlogConfig.mergeFromProperties()` has the same immutable-change check for it
but `checkBinlogConfigChange()` currently does not route it either.
- Keep/adjust the regression to assert that changing `binlog.format` on an
existing ROW-binlog table fails through the binlog config validator with `not
support change binlog format`, not through the generic row-binlog schema-change
guard.
Missing information:
- No additional logs are needed to identify the likely code-level cause for
the reported SQL.
- If maintainers want to pin the exact introducing commit, the public issue
should include a public branch/PR or full commit SHA reachable from
`apache/doris`; the reported short SHA was not found from the public repository
during this read-only check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
