✨ translate SQLite defaults to MySQL #142
Conversation
WalkthroughAdds MySQL/MariaDB feature-detection helpers and runtime flags; implements utilities to strip fully wrapping parentheses and translate SQLite DEFAULT expressions to MySQL-compatible defaults (CURRENT_*, numerics, booleans, strings, hex, expression handling with fractional-seconds support); integrates translation into table creation and adjusts column-length signature. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as Caller
participant T as SQLite3toMySQL
participant C as ColumnSpec
participant U as mysql_utils
CLI->>T: create_table(...)
T->>C: read type, dflt_value
T->>U: check_mysql_*_support(version)
Note right of T #DDEBF7: set runtime flags\n(_allow_expr_defaults, _allow_current_ts_dt, _allow_fsp)
T->>T: _translate_default_for_mysql(type, str(dflt_value))
T->>T: _strip_wrapping_parentheses(expr)
T-->>T: mapped MySQL default (maybe with precision)
T->>T: _column_type_length(type, default?)
T-->>CLI: emit column DDL with translated DEFAULT
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #142 +/- ##
==========================================
- Coverage 94.62% 93.85% -0.78%
==========================================
Files 8 8
Lines 689 797 +108
==========================================
+ Hits 652 748 +96
- Misses 37 49 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sqlite3_to_mysql/transporter.py (1)
498-510: Defaults like 0 or '' are silently dropped; also allow translator to suppress unsupported expressionsThe truthy check on dflt_value skips legitimate falsy defaults. Build the DEFAULT clause with a None check and only append when the translated default is non‑empty.
Apply this diff:
- sql += " `{name}` {type} {notnull} {default} {auto_increment}, ".format( + # Build DEFAULT clause safely (preserve falsy defaults like 0/'') + default_clause = "" + if ( + column["dflt_value"] is not None + and column_type not in MYSQL_COLUMN_TYPES_WITHOUT_DEFAULT + and not auto_increment + ): + td = self._translate_default_for_mysql(column_type, str(column["dflt_value"])) + if td != "": + default_clause = "DEFAULT " + td + sql += " `{name}` {type} {notnull} {default} {auto_increment}, ".format( name=mysql_safe_name, type=column_type, notnull="NOT NULL" if column["notnull"] or column["pk"] else "NULL", auto_increment="AUTO_INCREMENT" if auto_increment else "", - default=( - "DEFAULT " + self._translate_default_for_mysql(column_type, str(column["dflt_value"])) - if column["dflt_value"] - and column_type not in MYSQL_COLUMN_TYPES_WITHOUT_DEFAULT - and not auto_increment - else "" - ), + default=default_clause, )
🧹 Nitpick comments (2)
src/sqlite3_to_mysql/transporter.py (2)
356-450: Timezone modifiers may change semanticsSQLite’s "datetime/date/time('now','localtime'|'utc')" modifiers don’t map 1:1 to MySQL. CURRENT_* evaluates in the session time zone. Consider documenting this behavioural change or logging a warning when a timezone modifier is present.
451-459: Don’t truth‑test the default parameter; allow 0If default is 0, the current check skips it. Use an explicit None check.
Apply this diff:
- if default: + if default is not None: return f"({default})"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sqlite3_to_mysql/transporter.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
- GitHub Check: Codacy Static Code Analysis
…arious expressions
…, and fractional seconds support
…l seconds support
…TIME in default expressions
…various data types and expressions
…ctional seconds support
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/sqlite3_to_mysql/transporter.py (1)
360-389: Parentheses stripping now safely removes only fully wrapping pairsThis addresses the previously raised pitfall with "(a) + (b)". LGTM.
🧹 Nitpick comments (4)
src/sqlite3_to_mysql/mysql_utils.py (1)
142-164: Version‑gated capability checks look correct; consider DRYing MariaDB detectionThresholds mirror documented cut‑offs. To reduce repetition and future mistakes, extract a tiny helper, e.g.
_is_mariadb(ver: str) -> bool, and reuse it in all checkers.tests/unit/sqlite3_to_mysql_test.py (2)
670-710: DATETIME on old MySQL may still emit invalid DEFAULT; add failing caseWhen
ts_dt=False(MySQL < 5.6.5),_translate_default_for_mysqlcurrently falls through and returnsCURRENT_TIMESTAMPfor DATETIME, which MySQL will reject. Please add a case like:
- ("DATETIME(6)", "CURRENT_TIMESTAMP", dict(expr=False, ts_dt=False, fsp=True), "")
Also consider a couple more literals:
- Numeric edge forms: ".5", "1." (both valid in SQLite/MySQL) to ensure the numeric regex accepts them.
711-719: TIME fsp mapping covered; add localtime/utc variantsConsider adding:
- time('now','localtime') → CURRENT_TIME(2)
- time('now','utc') → CURRENT_TIME(2)
src/sqlite3_to_mysql/transporter.py (1)
65-77: Broaden numeric literal regex to match .5 and 1. formsYour current pattern doesn’t match literals like ".5" or "1." which both parse in SQLite/MySQL. Recommend this tweak.
Apply this diff:
- NUMERIC_LITERAL_PATTERN: t.Pattern[str] = re.compile(r"^[+-]?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?$") + NUMERIC_LITERAL_PATTERN: t.Pattern[str] = re.compile( + r"^[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][+-]?\d+)?$" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/sqlite3_to_mysql/mysql_utils.py(1 hunks)src/sqlite3_to_mysql/transporter.py(5 hunks)src/sqlite3_to_mysql/types.py(1 hunks)tests/unit/mysql_utils_test.py(2 hunks)tests/unit/sqlite3_to_mysql_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/sqlite3_to_mysql_test.py (1)
src/sqlite3_to_mysql/transporter.py (2)
_strip_wrapping_parentheses(361-389)_translate_default_for_mysql(391-486)
tests/unit/mysql_utils_test.py (1)
src/sqlite3_to_mysql/mysql_utils.py (3)
check_mysql_current_timestamp_datetime_support(150-155)check_mysql_expression_defaults_support(142-147)check_mysql_fractional_seconds_support(158-163)
src/sqlite3_to_mysql/transporter.py (1)
src/sqlite3_to_mysql/mysql_utils.py (3)
check_mysql_current_timestamp_datetime_support(150-155)check_mysql_expression_defaults_support(142-147)check_mysql_fractional_seconds_support(158-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
🔇 Additional comments (8)
src/sqlite3_to_mysql/types.py (1)
88-90: Attributes added are appropriate; ensure they’re always initialised before useThe three private flags align with the runtime feature checks and tests. Looks good; just make sure they’re set in every code path before any helper that relies on them is called (they are in SQLite3toMySQL.init, but please keep this invariant if future refactors introduce alternate constructors/factories).
tests/unit/mysql_utils_test.py (1)
8-10: Solid coverage of edge and vendor/version boundariesThe parametrisations exercise both MySQL and MariaDB thresholds (including distro suffixes and case). Nice.
Also applies to: 215-229, 230-241, 243-256, 257-267, 269-281, 282-292
tests/unit/sqlite3_to_mysql_test.py (2)
637-657: Parentheses stripper tests are tight and realisticGood mix of fully‑wrapped, partially wrapped and unmatched cases; these will guard against regressions.
658-669: Lightweight instance builder is handyThe flag toggles mimic runtime checks well and keep the tests fast.
src/sqlite3_to_mysql/transporter.py (4)
47-49: Good: centralised capability gatingImporting the three helpers keeps version logic in one place.
212-214: Flags wired correctly from capability checksThese match the new attributes and tests; nice and explicit.
489-495: Helper extension is fineDefaulting length via
defaultparameter simplifies PK length handling.
535-545: DEFAULT clause construction is robustGood: skips types that can’t have defaults, preserves falsy defaults (0/''), and avoids AUTO_INCREMENT columns.
Also applies to: 550-550
…fractional seconds support
…for clarity and consistency
…mpty string for non-expressions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/func/test_cli.py (2)
39-49: Add reason and make xfail strict (or fix root cause and keep test active).Bare xfail can mask regressions. Either stabilise the test by isolating env (preferred) or keep the xfail but make it explicit and strict.
Apply this minimal change:
- @pytest.mark.xfail + @pytest.mark.xfail(reason="Behaviour varies when CLI picks defaults from environment variables.", strict=True)If the flakiness is due to env-provided defaults, consider invoking with a clean environment instead of xfail:
result = cli_runner.invoke(sqlite3mysql, ["-f", sqlite_database], env={})Would you prefer a follow-up PR that removes xfail by isolating the environment for this test?
51-63: Same here: make xfail explicit and strict (or stabilise via env isolation).Mirror the change above to avoid silent XPASS; or drop xfail and pass an empty env to make the error deterministic.
Suggested tweak:
- @pytest.mark.xfail + @pytest.mark.xfail(reason="Behaviour varies when CLI picks defaults from environment variables.", strict=True)Optionally replace with:
result: Result = cli_runner.invoke( sqlite3mysql, ["-f", sqlite_database, "-d", mysql_credentials.database], env={} )Confirm whether the underlying issue is CI environment leakage (env vars). If so, I can prep a targeted fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sqlite3_to_mysql/transporter.py(5 hunks)tests/func/test_cli.py(2 hunks)tests/unit/sqlite3_to_mysql_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/sqlite3_to_mysql_test.py
- src/sqlite3_to_mysql/transporter.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
…ssions instead of empty strings
1 participant
This pull request significantly improves the handling of default column values during SQLite-to-MySQL schema migration, especially for date/time and boolean types, and enhances compatibility with various MySQL and MariaDB versions. The changes introduce robust translation of SQLite default expressions into MySQL-compatible defaults, with careful version checks for advanced features. Comprehensive tests ensure the new logic is correct and reliable.
Default value translation and version-aware feature support:
check_mysql_expression_defaults_support,check_mysql_current_timestamp_datetime_support,check_mysql_fractional_seconds_support) to detect support for expression defaults,CURRENT_TIMESTAMPforDATETIME, and fractional seconds in MySQL/MariaDB, with corresponding attributes inSQLite3toMySQLAttributes. [1] [2]Improved default value handling in schema translation:
_translate_default_for_mysqlinSQLite3toMySQLto convert SQLite default expressions (includingCURRENT_TIMESTAMP,datetime('now'), booleans, numeric literals, and quoted strings) into the correct MySQL syntax, with logic to handle version-specific support and fallback behavior.Regex and parsing enhancements:
Comprehensive testing:
Imports and code organization:
Fixes:
datetime('now')to MariaDBNOW()#141