fix(doris): re-raise genuine column DDL failures instead of swallowing them - #2359
fix(doris): re-raise genuine column DDL failures instead of swallowing them#2359AmirF194 wants to merge 2 commits into
Conversation
…g them _apply_table_actions wrapped every ALTER TABLE ADD/DROP COLUMN in a bare except Exception: pass, with no re-raise and no logging. Since _TableHandler.reconcile() commits the tracking record from the desired schema regardless of whether the DDL actually succeeded, a genuine failure (a permission error, an in-progress schema-change job, a syntax error) left the tracked schema state permanently desynced from the real table: the next incremental run saw prev == desired and never retried. Extract the per-column apply loop into _apply_column_actions and add _is_benign_column_ddl_error to distinguish an idempotent no-op (the column is already in the state the action wants) from a genuine failure, which is now logged and re-raised. Mirrors the sqlite connector's own handling of the same bug class. Fixes cocoindex-io#2358
|
Bumping this in case it slipped by, no rush. Happy to answer questions on the DDL reconciliation fix or adjust the approach. |
|
Hi @AmirF194 , I think it is LGTM with some minor fix expected as follow up. after rolled-back run -> column_actions {'col:col1': 'upsert'} # retried Also checked the matcher against a real Doris/VeloDB cluster instead of the strings in the tests, since those are circular by construction:
I expected "not exist" to over-match the missing-table case; it doesn't. One thing to fix before merge: these tests skip in CI. --group ci pulls ci-enabled-optional-deps (pydantic, asyncpg, neo4j), and pymysql/aiohttp only live in the doris extra, so DEPS_AVAILABLE=False and the module never runs — the green checks here don't cover it. Skipping is normal for python/tests/connectors/, but this is the first file there that doesn't need a live cluster, so adding pymysql + aiohttp to ci-enabled-optional-deps makes them real. (aiomysql is imported lazily in connect_async, so those two are enough.) Two follow-ups, not blockers — the same bug class in paths this PR didn't touch:
Worth a line in the PR body that a genuine failure now aborts the rest of the batch rather than continuing silently. That's the right trade and rollback makes it safe, but it's a user-visible change. |
The Doris DDL-swallow regression tests gate on pymysql/aiohttp (DEPS_AVAILABLE), which the ci dependency group never installed, so they silently skipped in CI. aiomysql is only imported lazily inside connect_async and is not needed for these tests.
|
Thanks for verifying the mechanism directly, and for catching the CI gap. Added pymysql and aiohttp to ci-enabled-optional-deps (aiomysql is only used lazily in connect_async, so those two are enough); confirmed locally that the suite skips all 10 without them and passes all 10 with them, matching what CI will now install. Left the two follow-ups (replace falling through with no DDL, and the swallowed DROP TABLE exception) out of this PR since they're a different code path than the one this fix touches, happy to open a separate issue for those if that's useful. |
Fixes #2358
Root cause
_apply_table_actionsinpython/cocoindex/connectors/doris/_target.pyapplied non-PK columnADD COLUMN/DROP COLUMNDDL inside a bareexcept Exception: pass. Because_TableHandler.reconcile()commits the tracking record from the desired schema regardless of whether the DDL succeeded, a genuine failure (a permission error, a schema-change job already in progress, a syntax error) was silently accepted as success: the next incremental run sawprev == desiredand never retried, leaving the tracked schema permanently out of sync with the real table.Fix
Extracted the per-column apply loop into
_apply_column_actionsand added_is_benign_column_ddl_error, which distinguishes an idempotent no-op (the column is already in the state the action wants) from a genuine failure. A benign error is still ignored; anything else is logged and re-raised, so the caller'sreconcile()does not commit a tracking record for a change that never happened. This mirrors how the sqlite connector already handles the identical failure mode for its ownADD COLUMN.Doris' DDL dialect does not appear to support
ADD COLUMN IF NOT EXISTS/DROP COLUMN IF EXISTS(unlike postgres, which sidesteps this ambiguity entirely with those clauses), so the fix classifies the error message rather than changing the SQL, the same approach sqlite's connector takes forduplicate column name.Verification
This repo's own Doris tests need a live cluster (
DORIS_FE_HOST/DORIS_PASSWORD), which I don't have, so I addedpython/tests/connectors/test_doris_target_ddl_swallow.py, which exercises the real control flow without one: it calls_apply_column_actionsdirectly, and an end-to-end test calls the actual_apply_table_actionsentry point with a stubbed_execute_ddl_sync,ContextProvider, andManagedConnection(no Doris connection ever opens).main, the end-to-end one withDID NOT RAISE Exception(confirming the swallow, not just missing symbols), and pass against this branch. Ran both ways in the same container (python:3.11, matching this repo's own CI matrix's standard leg) againstcocoindex==1.0.20's compiled core with this repo's Python source overlaid on top.ruff checkandruff format --checkon both changed files: clean, pinned to this repo's ownv0.12.0.mypystrict over the wholepython/tree (uv run mypy): clean, 281 files.ADD/DROP COLUMNconflict. I don't have one available, and this repo's own connector tests are gated onDORIS_FE_HOSTfor the same reason. I did not attempt a localmaturin developbuild (this repo's realpytestCI hook) since a full Rust workspace build was not practical in my environment; the tests above ran against the officially published wheel's compiled core with this branch's Python source substituted in, exercising the exact code path unmodified.