fix: stop declaring a column type the storage will not match - #239
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesThe change updates numeric detection and column type inference. Go-only numeric syntax remains Column type detection
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@types.go`:
- Around line 662-665: Remove the claim that SQLite rejects digit-separating
underscores, while preserving the explanation that imported or bound text
literals can be rejected by numeric affinity conversion and stored as text.
Apply this wording update in types.go:662-665, types_test.go:716-718, and
CHANGELOG.md:12; update each site consistently, with no other behavior changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b40fbd36-2ddc-4acb-ba66-b16e149c6115
📒 Files selected for processing (3)
CHANGELOG.mdtypes.gotypes_test.go
Code Metrics Report
Details | | main (0e13465) | #239 (e49b2d9) | +/- |
|---------------------|----------------|----------------|-------|
- | Coverage | 89.9% | 89.9% | -0.1% |
| Files | 58 | 58 | 0 |
| Lines | 9991 | 10000 | +9 |
+ | Covered | 8985 | 8992 | +7 |
+ | Test Execution Time | 18s | 13s | -5s |Code coverage of files in pull request scope (92.9% → 92.4%)
Reported by octocov |
Summary
Column type detection could declare
INTEGERorREALfor a column whose values SQLite then stored as text, so the schema a caller reads andtypeof()disagreed with no way to tell from the outside. Two independent triggers, both fixed here.Changes
types.go:isFloatrejects numeric syntax that only Go accepts, via a newhasGoOnlyNumericSyntaxhelper.strconv.ParseFloattakes a digit-separating underscore (1_000,1_000.5) and a hexadecimal float (0x1p4); SQL takes neither, so those values were called REAL and stored as text.types.go:selectColumnTypereturns text when a column holds a datetime alongside a number. A datetime is stored as text, so there is no type covering both, which is the rule the function already applied to text values.types_test.go: cases for both triggers inTestIsFloatandTestSelectColumnType.CHANGELOG.md.Design Decisions
The rule for text was already at the top of
selectColumnType: if any value in the column is text, the column is text, because no other type holds it. A datetime value is stored as text too, so the same reasoning applies and the fix is that rule extended rather than a new confidence tweak. Going by confidence instead is what let three integers and four reals outvote two datetimes and declare REAL over values that were never numeric.For the literal syntax the check is a small explicit test rather than a stricter regex over the whole value, because
ParseFloatis otherwise the right acceptor and only these two Go-only spellings need excluding. Binary (0b101) and octal (0o17) literals already failedParseFloatand need no handling.Limitations
One existing
TestSelectColumnTypecase pinned the old answer: three integers, four reals, and two datetimes returned REAL. That is the behavior being fixed, so its expectation is now text, and a datetime-free version of the case keeps covering the numeric fallback it was written for.Summary by CodeRabbit
Bug Fixes
TEXT, matching SQLite behavior.Tests