[eschutho]

SUMMARY

Flask-AppBuilder's DB authentication resolves users case-insensitively. With
AUTH_USERNAME_CI enabled (the Flask-AppBuilder default), the username branch
of SecurityManager.find_user() — and Superset's find_user_with_relationships
in superset/security/manager.py — folds case on both sides of the comparison,
so on essentially every authenticated request it runs:

SELECT ab_user.* FROM ab_user WHERE lower(ab_user.username) = lower(?)

ab_user only carries a plain, case-sensitive unique btree on username. That
index is keyed on the raw column value, so it cannot satisfy a lower(username)
predicate — the expression is evaluated per row and the planner falls back to a
sequential scan of ab_user. On deployments with a large user table and a high
volume of authenticated requests this is a recurring, avoidable cost on the
metadata database.

This migration adds a functional index on lower(username) so the same lookup
becomes an index probe.

Why only username and not email: the email-based lookup path is an exact
match (Flask-AppBuilder's filter_by(email=email) / Superset's email == email)
with no lower() wrapping, so the existing plain email index already serves it.
A lower(email) index would be dead weight and is intentionally omitted.

Migration mechanics (dialect-specific):

• PostgreSQL (online): CREATE INDEX CONCURRENTLY IF NOT EXISTS inside an
  autocommit_block(). Because ab_user is read on nearly every request, a
  plain CREATE INDEX would take an exclusive lock on the live table for the
  duration of the build; CONCURRENTLY avoids that, at the cost of not being
  runnable inside a transaction (hence the autocommit block). IF NOT EXISTS
  keeps re-runs idempotent.
• PostgreSQL (offline / --sql): CONCURRENTLY / autocommit_block()
  cannot be used when Alembic only emits SQL, so a plain
  CREATE INDEX IF NOT EXISTS is generated instead, keeping
  superset db upgrade --sql valid.
• SQLite: CREATE INDEX IF NOT EXISTS over lower(username).
• MySQL / other: a functional key part requires the doubled parentheses
  ((lower(username))), and stock MySQL has no IF NOT EXISTS grammar for
  CREATE INDEX, so the inspector is probed first and creation is skipped when
  the index already exists.

downgrade() mirrors this (Postgres online DROP INDEX CONCURRENTLY IF EXISTS
in an autocommit block, offline plain DROP INDEX IF EXISTS, other dialects
probe the inspector before dropping).

Note for reviewers: This is, as far as I can tell, the first Superset
migration to use CREATE INDEX CONCURRENTLY / autocommit_block(). I'm
opening it as a draft so reviewers can confirm the non-transactional
autocommit approach is acceptable for the project's migration tooling and for
offline (--sql) SQL generation before it lands.

TESTING INSTRUCTIONS

• Run superset db upgrade on PostgreSQL, SQLite and MySQL and confirm the
  ix_ab_user_lower_username index is created on ab_user.
• On PostgreSQL, verify EXPLAIN of
  SELECT * FROM ab_user WHERE lower(username) = lower('someuser') uses the new
  index instead of a sequential scan.
• Run superset db upgrade --sql (offline mode) on PostgreSQL and confirm the
  generated SQL contains a plain CREATE INDEX ... (lower(username)) (no
  CONCURRENTLY) and is valid.
• Run superset db downgrade and confirm the index is dropped.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.81%. Comparing base (fe1b368) to head (6db29f7).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #43939      +/-   ##
==========================================
- Coverage   79.81%   79.81%   -0.01%     
==========================================
  Files        2913     2913              
  Lines      170345   170345              
  Branches    39491    39491              
==========================================
- Hits       135958   135954       -4     
- Misses      31865    31869       +4     
  Partials     2522     2522              

Flag	Coverage Δ
hive	37.62% <ø> (ø)
mysql	57.33% <ø> (ø)
postgres	57.36% <ø> (ø)
presto	39.50% <ø> (ø)
python	84.21% <ø> (ø)
sqlite	57.07% <ø> (ø)
unit	75.19% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codeant-ai-for-open-source]

Suggestion: The catch-all branch emits MySQL functional-index syntax on older MySQL and MariaDB servers, where this statement fails and blocks the migration. [api mismatch]

Assessment: 🔴 Critical · 🔁 Occurrence: Sometimes

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset/migrations/versions/2026-09-05_00-00_88a01c781622_index_ab_user_lower_username.py
**Line:** 123:124
**Comment:**
	*Api Mismatch: The catch-all branch emits MySQL functional-index syntax on older MySQL and MariaDB servers, where this statement fails and blocks the migration.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[bito-code-review]

The flagged issue is correct. Older versions of MySQL (pre-8.0.13) and MariaDB (pre-10.2) do not support functional indexes, and the syntax ((lower(username))) will cause a syntax error on these versions, blocking the migration.

To resolve this, you should detect the database version or dialect and conditionally apply the index creation. Since Superset supports various environments, a common approach is to check the database version or use a try-except block around the execution. However, given the current implementation, a safer approach is to ensure the migration only attempts this on supported versions or provides a fallback.

Would you like me to implement a version check for MySQL/MariaDB to prevent this error on older servers?

superset/migrations/versions/2026-09-05_00-00_88a01c781622_index_ab_user_lower_username.py

else:
        # MySQL and any other dialect: no portable IF NOT EXISTS for
        # CREATE INDEX, so probe the inspector and skip if the index is
        # already present. A functional key part requires the doubled
        # parentheses: ((lower(username))).
        if not _index_exists(bind):
            op.execute(f"CREATE INDEX {INDEX_NAME} ON {TABLE_NAME} ((lower(username)))")

[aminghadersohi]

Reviewed at 6db29f7f1757848d2e1a2ad577589125d3a9aaf9. Thanks for this — the docstring is genuinely excellent (the reasoning about why only username and not email is indexed is exactly the kind of thing that saves the next reader an hour), and the core premise checks out.

What I verified as correct (RAN, not just read):

• The premise holds. Both FAB's BaseSecurityManager.find_user and Superset's find_user_with_relationships (superset/security/manager.py:4990) emit lower(username) = lower(?) under auth_username_ci, and AUTH_USERNAME_CI defaults to True in FAB 5.2.1. On a 2M-row ab_user in PostgreSQL 17.9 the lookup goes from Parallel Seq Scan to Bitmap Index Scan on ix_ab_user_lower_username with this index in place. On SQLite, EXPLAIN QUERY PLAN gives SEARCH ab_user USING INDEX ix_ab_user_lower_username (<expr>=?). The optimisation is real.
• Migration chain is clean. 385 revisions, down_revision = "7e2c9a4f1b83" exists, zero missing parents, and 88a01c781622 is the single head.
• CI is green. Deduped newest-per-name across the full rollup: 67 legs, 51 SUCCESS / 13 SKIPPED / 3 NEUTRAL, zero failures.
• SQLite branch is fully idempotent — ran upgrade() twice and downgrade() twice against a live in-memory engine, all clean.

Three things I think need attention — details inline. Two are confirmed against live database servers, one is a crash I hit while exercising offline mode.

1. codeant's Critical is correct, and I'd treat it as the blocker. I reproduced the failure on MariaDB 10.11.19 and MySQL 5.7.44 — both ERROR 1064 on the exact statement this branch emits. More importantly, this repo already has the answer: 2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py:79 added _mysql_supports_functional_index() for precisely this, and UPDATING.md:1159 documents MySQL <8.0.13 / MariaDB as backends that must be gated rather than assumed away. So this isn't a hypothetical-dialect concern — it's an established convention in the migration directory with a helper already written.
2. CREATE INDEX CONCURRENTLY + IF NOT EXISTS is not idempotent in the failure case (the docstring at line 108 claims it is). Confirmed on PostgreSQL 17.9 — a partially-failed concurrent build leaves an invalid index behind, and the re-run then skips it forever. Details and a verified fix inline.
3. Offline --sql mode crashes on MySQL with NoInspectionAvailable. The Postgres path handles ctx.as_sql; the else path calls the inspector unconditionally. Inline.

Also worth flagging: bito's reply is right in conclusion but wrong in a detail that matters — it says MariaDB "pre-10.2" lacks functional indexes, which implies 10.2+ is fine. My run on 10.11.19 fails; MariaDB has no expression-index support at any 10.x, which is exactly why the precedent helper excludes it by is_mariadb rather than by version.

One thing I deliberately am not suggesting: making this index UNIQUE. ab_user already carries a plain unique index on the raw username, so deployments may legitimately hold case-colliding rows (Admin / admin), and a unique functional index would fail to build on exactly those and block the upgrade. The resulting find_user() ambiguity is pre-existing and out of scope here — noting it only as context, not as a requested change.

A hypothesis I had and disproved, since a wrong warning would cost you time: I expected _index_exists() to be blind to functional indexes on MySQL (SQLAlchemy does skip them on SQLite — SAWarning: Skipped unsupported reflection of expression-based index, and get_indexes() returns []). On MySQL it works fine: the SHOW CREATE TABLE parser returns the key with the correct name and empty column_names, and MySQLDialect.get_indexes does not filter empty-column entries, so the name match succeeds. The MySQL re-run guard is sound — no change needed there.

Testing: tests/unit_tests/migrations/ has direct precedent — test_add_deleted_at_to_dashboards.py covers the analogous functional-index migration including test_upgrade_is_idempotent / test_downgrade_is_idempotent / a dialect no-op case. A small test in that shape would lock down the idempotency claims this migration makes in its own docstring. Non-blocking, but cheap given the harness already exists.

UPDATING.md: nothing owed for adding an index as such. But once the dialect gating in (1) lands, the behaviour genuinely differs per backend, and the precedent migration documented exactly that in UPDATING.md:1159. Worth a short paragraph for consistency — also a good place to mention that the online Postgres path uses CONCURRENTLY (first use in this repo — grep finds no other CONCURRENTLY or autocommit_block in superset/), which has real operational implications for large ab_user tables.

Not approving here, but only because there's an unresolved review thread at this head — resolving another reviewer's thread isn't mine to do. Once (1) and (2) are addressed and that thread is closed, I don't see anything else standing between this and an approve.

[aminghadersohi]

HIGH — confirmed: this hard-fails on MySQL <8.0.13 and on MariaDB at any version. (Agreeing with codeant's Critical here — I reproduced it rather than taking it on faith.)

Ran the exact statement this line emits against live servers:

MariaDB 10.11.19: ERROR 1064 (42000) ... near '(lower(username)))' at line 1
MySQL   5.7.44  : ERROR 1064 (42000) ... near '(lower(username)))' at line 1

Functional key parts landed in MySQL 8.0.13; MariaDB has no expression-index support at all (it wants a generated column), so is_mariadb has to be excluded by flavour, not by version number. On those backends superset db upgrade aborts here and the deployment cannot move forward.

This isn't a speculative-dialect worry — the repo already settled it. 2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py:79 added exactly the helper needed:

def _mysql_supports_functional_index(bind: Connection) -> bool:
    if getattr(bind.dialect, "is_mariadb", False):
        return False
    return (bind.dialect.server_version_info or ()) >= (8, 0, 13)

and UPDATING.md:1159 documents the resulting matrix ("MySQL <8.0.13, MariaDB, and SQLite keep the original full unique constraint"). Suggest mirroring that shape:

    elif dialect == "mysql" and _mysql_supports_functional_index(bind):
        if not _index_exists(bind):
            op.execute(f"CREATE INDEX {INDEX_NAME} ON {TABLE_NAME} ((lower(username)))")
    # else: no functional-index support -> skip; the lookup keeps its
    # existing (seq-scan) behaviour, which is what it does today anyway.

Two secondary benefits of restructuring it this way: skipping is strictly a no-op relative to current behaviour (no regression for anyone), and it removes the else catch-all that currently sends MySQL-specific ((...)) grammar to any unrecognised dialect.

The downgrade() catch-all at line 145 needs the same treatment, otherwise it will try to drop an index that was never created.

[aminghadersohi]

HIGH — the idempotency claim in this comment does not hold in the one failure mode CONCURRENTLY actually introduces. Confirmed against PostgreSQL 17.9.

When CREATE INDEX CONCURRENTLY fails partway through — cancelled session, deadlock, a deploy job that times out, or (the easiest one to hit in production) a server-side statement_timeout — PostgreSQL leaves an invalid index behind rather than rolling it back. IF NOT EXISTS then matches on name only, so every subsequent run skips it.

Reproduced end to end on a 2M-row ab_user:

-- run 1, with statement_timeout=60ms
ERROR:  canceling statement due to statement timeout

SELECT c.relname, i.indisvalid FROM pg_index i JOIN pg_class c ON c.oid=i.indexrelid ...;
 ix_ab_user_lower_username | f      <-- invalid index now exists

-- run 2, the exact statement this migration emits, no timeout
CREATE INDEX
NOTICE:  relation "ix_ab_user_lower_username" already exists, skipping
 ix_ab_user_lower_username | f      <-- still invalid

EXPLAIN (COSTS OFF) SELECT * FROM ab_user WHERE lower(username)=lower('U500000');
 Gather
   ->  Parallel Seq Scan on ab_user

The end state is the bad one: the migration reports success, Alembic stamps the revision so superset db upgrade won't retry, pg_indexes lists the index so any name-based check (including _index_exists()) says it's present — and the planner never uses it, so the entire performance goal of this PR is silently unmet on that deployment. It's a dead index that also costs write amplification.

Suggested fix — drop an invalid leftover before (re)creating. Verified working on the same instance:

with ctx.autocommit_block():
    # A previously interrupted CONCURRENTLY build leaves an INVALID index
    # behind; IF NOT EXISTS matches on name alone and would skip forever.
    op.execute(
        f"""
        DO $$
        BEGIN
            IF EXISTS (
                SELECT 1 FROM pg_index i
                JOIN pg_class c ON c.oid = i.indexrelid
                WHERE c.relname = '{INDEX_NAME}' AND NOT i.indisvalid
            ) THEN
                EXECUTE 'DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}';
            END IF;
        END $$;
        """
    )
    op.execute(
        f"CREATE INDEX CONCURRENTLY IF NOT EXISTS {INDEX_NAME} "
        f"ON {TABLE_NAME} (lower(username))"
    )

(DROP INDEX CONCURRENTLY can't run inside DO/a transaction block — if that trips, splitting into a SELECT probe then a conditional op.execute from Python works and is arguably easier to read. Either way the check is pg_index.indisvalid, not existence.)

After drop-and-rebuild the same instance reports indisvalid | t and plans a Bitmap Index Scan on ix_ab_user_lower_username — so the fix is confirmed, not just plausible.

Worth a line in the docstring too: "IF NOT EXISTS keeps the migration idempotent across re-runs" is true for the success path and misleading for the failure path, which is the one that matters here.

[aminghadersohi]

MEDIUM — offline --sql generation crashes on MySQL. Found while exercising the dialect branching; RAN:

--- postgresql OFFLINE ---
CREATE INDEX IF NOT EXISTS ix_ab_user_lower_username ON ab_user (lower(username));
DROP INDEX IF EXISTS ix_ab_user_lower_username;
--- sqlite OFFLINE ---
CREATE INDEX IF NOT EXISTS ix_ab_user_lower_username ON ab_user (lower(username));
DROP INDEX IF EXISTS ix_ab_user_lower_username;
--- mysql OFFLINE RAISED: NoInspectionAvailable: No inspection system is
    available for object of type <class 'sqlalchemy.engine.mock.MockConnection'>

In offline mode op.get_bind() returns a MockConnection, and _index_exists() calls sa.inspect(bind) on it unconditionally — there's no live connection to reflect from. The Postgres path already branches on ctx.as_sql (line 94), so offline mode is clearly in scope for this migration; the else path just doesn't get the same treatment. downgrade() has the identical problem at line 145.

Since MySQL has no IF NOT EXISTS grammar for CREATE INDEX, the honest offline output is the bare statement plus a note that the operator must check first:

    elif dialect == "mysql" and (ctx.as_sql or _mysql_supports_functional_index(bind)):
        if ctx.as_sql:
            # Offline: cannot probe the inspector. Emit the statement bare;
            # MySQL has no IF NOT EXISTS for CREATE INDEX, so re-applying the
            # generated script against a database that already has the index
            # will error -- intentional, and visible to the operator.
            op.execute(f"CREATE INDEX {INDEX_NAME} ON {TABLE_NAME} ((lower(username)))")
        elif not _index_exists(bind):
            op.execute(f"CREATE INDEX {INDEX_NAME} ON {TABLE_NAME} ((lower(username)))")

(Note this interacts with the version gate in my other comment: in offline mode there's no server_version_info either, so the generated script is necessarily "assume a supported server" — worth saying so in a comment, since the operator is the one who knows their server version.)

[bito-code-review]

Code Review Agent Run #7216b2

Actionable Suggestions - 0

Additional Suggestions - 1

• superset/migrations/versions/2026-09-05_00-00_88a01c781622_index_ab_user_lower_username.py - 1

  • Offline mode crashes on MySQL · Line 123-123
    In offline mode (`alembic upgrade --sql`) `op.get_bind()` returns a MockConnection, so `_index_exists(bind)` → `inspector.get_indexes()` runs a SELECT that fails (Alembic offline docs: SELECT-based ops don't work in `--sql`). The PostgreSQL branch guards `ctx.as_sql`, but this MySQL/other branch (and the downgrade else-branch) does not, so offline SQL generation for MySQL crashes. Emit the plain CREATE/DROP INDEX when `ctx.as_sql`.

Review Details

• Files reviewed - 1 · Commit Range: 6db29f7..6db29f7

  • superset/migrations/versions/2026-09-05_00-00_88a01c781622_index_ab_user_lower_username.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by