Skip to content

fix(ddl): ALGORITHM/LOCK clause no longer discards subsequent ALTER operations (#1140) - #1381

Open
minguyen9988 wants to merge 3 commits into
Altinity:2.10.0from
minguyen9988:fix/alter-algorithm-drops-subsequent-operations
Open

fix(ddl): ALGORITHM/LOCK clause no longer discards subsequent ALTER operations (#1140)#1381
minguyen9988 wants to merge 3 commits into
Altinity:2.10.0from
minguyen9988:fix/alter-algorithm-drops-subsequent-operations

Conversation

@minguyen9988

Copy link
Copy Markdown
Collaborator

Problem

MySqlDDLParserListenerImpl.enterAlterTable walks the ALTER clause list and emits ClickHouse DDL per clause. On encountering AlterBySetAlgorithmContext it logged a message and break-ed out of the walk.

ALGORITHM= is a MySQL-only execution hint with no ClickHouse equivalent, so emitting nothing for it is correct — but abandoning the whole walk is not. Every operation positioned after the hint was silently discarded.

The statement from #1140:

ALTER TABLE test_lot ADD COLUMN event_ref_type_id INTEGER, algorithm=instant,
                     ADD COLUMN event_ref_id BIGINT, algorithm=instant;

generated only:

ALTER TABLE db.test_lot ADD COLUMN event_ref_type_id Nullable(Int32)

event_ref_id is never created in ClickHouse.

Why this is worse than a DDL gap

The failure is silent on every side. MySQL applies both columns, the connector raises no error, and the destination table simply lacks the column. Because the INSERT column list is built by iterating the ClickHouse column map, every subsequent row silently drops that column's value. There is no error, no warning, and no signal that the two schemas have diverged — the data loss is discovered later, by a checksum job or by a user noticing a blank column.

LOCK= is the same class of MySQL-only hint. It was falling through to the no-op terminal branch, leaving the separator that preceded it dangling in the generated statement — e.g. ... Nullable(Int32),, ADD COLUMN ..., which ClickHouse rejects outright.

Fix

Both hints now skip their own clause and continue the walk, dropping the now-orphaned separator via a small removeTrailingComma() helper.

A hint in trailing position is handled too: the comma sweep runs once more after the loop, so ..., ALGORITHM=INPLACE, LOCK=NONE leaves no dangling separator.

The existing behaviour for a trailing ALGORITHM clause is unchanged — that case already worked, and its existing tests still pass.

Tests

Three regression cases added to MySqlDDLParserListenerImplTest:

Test Covers
testAlterAddColumnWithInterleavedAlgorithmClauses the exact statement from #1140
testAlterAddColumnWithInterleavedLockClause LOCK= in non-trailing position
testAlterAddColumnWithTrailingHintsLeavesNoDanglingComma trailing hints emit no dangling separator

Verified failing before the change and passing after — not merely passing after:

WITHOUT the fix:  Tests run: 3, Failures: 2
  testAlterAddColumnWithInterleavedAlgorithmClauses
    expected: <...e_id nullable(int32)[, add column event_ref_id nullable(int64)]>
    but was:  <...e_id nullable(int32)[]>
  testAlterAddColumnWithInterleavedLockClause
    expected: <...col nullable(int32),[] add column second_c...>
    but was:  <...col nullable(int32),[,] add column second_c...>

WITH the fix (full class):  Tests run: 108, Failures: 0, Errors: 0, Skipped: 1

The full MySqlDDLParserListenerImplTest class passes, so no existing parser behaviour regressed.

How this was found

End-to-end verification of the 2.10.0 rollup against a MySQL → connector → ClickHouse test environment, comparing values (not row counts) across a DDL/DML matrix. This defect reproduced identically on 2.8.0 and 2.10.0.

Fixes #1140

…perations

enterAlterTable walks the ALTER clause list and emits ClickHouse DDL per
clause. On encountering AlterBySetAlgorithmContext it logged a message and
broke out of the walk. ALGORITHM= is a MySQL-only execution hint with no
ClickHouse equivalent, so emitting nothing for it is correct -- but abandoning
the whole walk is not: every operation positioned after the hint was silently
discarded.

    ALTER TABLE test_lot ADD COLUMN event_ref_type_id INTEGER, algorithm=instant,
                         ADD COLUMN event_ref_id BIGINT, algorithm=instant;

generated only

    ALTER TABLE db.test_lot ADD COLUMN event_ref_type_id Nullable(Int32)

event_ref_id was never created in ClickHouse. The failure is silent on both
sides: MySQL applies both columns, the connector raises no error, and
subsequent inserts drop the missing column value because the INSERT column
list is built from the ClickHouse schema.

LOCK= is the same class of MySQL-only hint and is handled identically -- it
previously fell through to the no-op branch, leaving the separator that
preceded it dangling in the generated statement.

Both hints now skip their own clause and continue the walk, dropping the
now-orphaned separator. A trailing hint is also handled: the comma sweep runs
once more after the loop, so a hint in final position leaves no dangling
separator.

Tests: three regression cases in MySqlDDLParserListenerImplTest -- the exact
statement from the issue report, the LOCK equivalent, and a trailing-hint case
asserting no dangling separator. Verified failing before the change (the first
case loses the second column entirely, the LOCK case emits a doubled comma)
and passing after. Full class: 108 run, 0 failures, 1 skipped.

Fixes Altinity#1140
@minguyen9988 minguyen9988 changed the title WIP: DO NOT MERGE - fix(ddl): ALGORITHM/LOCK clause no longer discards subsequent ALTER operations (#1140) fix(ddl): ALGORITHM/LOCK clause no longer discards subsequent ALTER operations (#1140) Aug 14, 2026
minguyen9988 and others added 2 commits August 14, 2026 11:14
…sent

Included so this PR gets real test signal: without it the fork-PR pipeline
dies at the Docker Hub push before any test runs, and every downstream job
is SKIPPED. Same change as the standalone CI PR; drop this commit if that
one lands first.

GitHub withholds repository secrets from fork-triggered workflows, so
DOCKERHUB_USERNAME is empty and the guarded login step is skipped. The
lightweight image build that follows is unguarded and runs
docker buildx build ... --push, which then reaches Docker Hub
unauthenticated and fails with 401 insufficient scopes.

The Kafka image above already handles this correctly -- it builds locally
and pushes in a separate step guarded by the same condition as the login.
This applies the same treatment to the lightweight image: the multi-arch
push build is gated on DOCKERHUB_USERNAME being non-empty, and a second
build gated on the inverse uses --load so the build is still validated and
the tarball the downstream test jobs consume is still produced.

The credentialed path is unchanged.

The file uses GitHub Actions expression syntax, not Jinja: it contains no
{% %} or {# #} markers, so it renders to itself. Verified by parsing the
artifact with a duplicate-key-detecting YAML loader (no duplicates, no
unrendered markers, non-empty) and confirming the two new build steps carry
mutually exclusive conditions, so exactly one runs in either case.

Jinja-Render-Check: rendered=1; formats=yaml; result=pass
Fork-originated pull requests do not receive repository secrets. Two
reporting steps use them unconditionally and fail the job *after* the
work they report on has already succeeded, so a fully green test run is
reported as a red check.

Upload artifacts to Altinity Test Reports S3 bucket
  Runs `aws s3 cp` with AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY. Its
  existing condition tests only for fork-ness, which selects exactly the
  runs where those secrets are empty, so on a fork PR the upload can
  only ever fail. Observed on PR Altinity#1383, whose diff touches nothing but
  .github/workflows/docker-build.yml: steps 1-12 including "Run testflows
  tests" all succeed and step 13 "Upload artifacts to Altinity Test
  Reports S3 bucket" fails the job.

  The condition now also requires the credential to be present, matching
  how docker-build.yml already gates its registry login on
  DOCKERHUB_USERNAME. AWS_ACCESS_KEY_ID is surfaced at workflow level so
  the step's own `if:` can read it -- a step's own `env:` block is not
  available to that step's `if:` expression. The credentialed path is
  unchanged. The artefacts remain attached to the run by the
  upload-artifact step that follows, so nothing is lost on forks.

Publish Test Report
  mikepenz/action-junit-report@v4 creates a check run, which a fork PR's
  read-only GITHUB_TOKEN cannot do; the step dies with "Failed to create
  checks using the provided token. (HttpError: Resource not accessible by
  integration)" and masks the very test result it was asked to report.
  It now falls back to annotations on forks. fail_on_failure stays
  enabled on both paths: a genuine test failure must still fail the job.

Affected: testflows-sink-connector-lightweight.yml (2 steps),
testflows-sink-connector-lightweight-arm.yml (2 steps),
testflows-sink-connector-kafka.yml (1 step),
sink-connector-lightweight-tests.yml (1 step).

Verified: all four workflows parse as YAML, and every `aws s3 cp` step in
the tree is confirmed to carry the credential guard.

Jinja-Render-Check: rendered=4; formats=yaml; result=pass
(cherry picked from commit 67d05e6)
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.

1 participant