airborne12 opened a new pull request, #69445:
URL: https://github.com/apache/doris/pull/69445
### What problem does this PR solve?
Issue Number: None
Related PR: #68674
Problem Summary:
`SchemaChangeHandler#process` handles the clauses of an ALTER TABLE
statement one by one, and some clauses update live table metadata in place
while they are processed:
- `processDropColumn` sets the table's bloom filter columns
(`olapTable.setBloomFilterInfo`);
- `removeColumnWhenDropGeneratedColumn` removes a dropped generated column
from the `generatedColumnsThatReferToThis` set of the live base schema columns
it reads;
- `processDropSequenceMapping` removes the column from the table's sequence
mapping group, because the mapping was copied shallowly and the groups are the
table's own lists.
When a later clause, or a check after the clause loop, fails the statement,
these updates stay on the master without an edit log, so the master diverges
from the followers (and the divergence is written into the next image):
```sql
-- t has b INT and g INT AS (b + 1); missing_idx does not exist
ALTER TABLE t DROP COLUMN g, DROP INDEX missing_idx; -- fails, but b no
longer lists g, so DROP COLUMN b is allowed later
-- v2 is a bloom filter column
ALTER TABLE t DROP COLUMN v2, DROP INDEX missing_idx; -- fails, but v2 is
no longer a bloom filter column
-- sequence_mapping.s1 = 'c,d'
ALTER TABLE t DROP COLUMN c, DROP INDEX missing_idx; -- fails, but s1 is
'd' in memory
```
In addition, `ADD INDEX IF NOT EXISTS` of an existing index and `DROP INDEX
IF EXISTS` of a missing index returned from `process()`, so the whole statement
became a no-op that reported success:
```sql
ALTER TABLE t DROP COLUMN x, DROP INDEX IF EXISTS missing_idx; -- returned
success, x was not dropped
```
Fix:
- The sequence mapping groups are copied as well, so the table's mapping
only changes through the schema change job.
- Before the clause loop, `process()` takes a snapshot of the bloom filter
columns and of the generated column references of the base schema columns (by
object identity, since `Column` equality is by value). If the statement ends
before it publishes its changes, by an exception or a return, the `finally`
block restores them. The statement counts as published right before it calls
`modifyTableLightSchemaChange`, `buildOrDeleteTableInvertedIndices` or
`createJob`.
- `IF NOT EXISTS` / `IF EXISTS` only skip their own clause; a statement
whose clauses are all skipped still succeeds without changes.
Validation that runs inside `createJob` / `modifyTableLightSchemaChange`
after the publish point is not covered by the restore. Making `process()` fully
atomic needs a staged rewrite that builds and validates the whole change on
copies before publishing it, which is left for a follow-up.
### Release note
A failed ALTER TABLE no longer leaves bloom filter columns, sequence mapping
groups or generated column dependencies changed in memory. `ADD INDEX IF NOT
EXISTS` / `DROP INDEX IF EXISTS` that do nothing no longer skip the other
clauses of the statement.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] 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 <!-- Add your reason? -->
`SchemaChangeHandlerTest#testFailedAlterKeepsLiveColumnMetadata` runs the
three failing statements above and checks the generated column references,
bloom filter columns and sequence mapping group are unchanged.
`testIfExistsSkipsOnlyItsClause` checks statements with only skipped clauses
still succeed and `DROP COLUMN x, DROP INDEX IF EXISTS missing_idx` drops `x`.
Both tests fail without the change.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
`IF NOT EXISTS` / `IF EXISTS` index clauses that do nothing no longer
turn the whole statement into a no-op.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
