bxsx commented on PR #42494: URL: https://github.com/apache/superset/pull/42494#issuecomment-5205063576
> ### Code Review Agent Run #866d34 > Actionable Suggestions - 0 > Additional Suggestions - 1 > * superset/migrations/versions/2026-07-28_10-00_2d6ad72e4af6_add_include_cta_to_report_schedule.py - 1 > > * Non-standard boolean server_default · [Line 47-47](https://github.com/apache/superset/pull/42494/files#diff-b349be099c04c86bc6211c0388815ebe52949bd08c311182d002674560007fc6R47) > Using `sa.true()` as a `server_default` is not the established pattern in this codebase. Other migrations use `expression.true()` for boolean defaults. If `sa.true()` doesn't translate correctly to SQL for existing rows, existing `report_schedule` records may get NULL instead of TRUE, violating the documented contract that 'NULL is treated as true'. `sa.true()` and `expression.true()` are the **same** function object — `sa.true is expression.true` evaluates to `True` (the former is just the top-level re-export), and both compile to `true`, so there's no translation risk for existing rows. As for the "established pattern": the migration set contains **exactly one** `expression.true()` (dating from 2018) versus 11 `sa.`-prefixed uses (`sa.false()` ×10, `sa.true()` ×1) - the `sa.` spelling is the established pattern here, and it avoids an extra import on top of the already-imported `sa`. Keeping `sa.true()`. -- 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]
