fix(csv): open input files as utf-8-sig to strip BOM transparently#69
fix(csv): open input files as utf-8-sig to strip BOM transparently#69gkorland wants to merge 1 commit into
utf-8-sig to strip BOM transparently#69Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 53 seconds.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #69 +/- ##
=======================================
Coverage 89.50% 89.50%
=======================================
Files 9 9
Lines 610 610
=======================================
Hits 546 546
Misses 64 64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Copilot Code Review✅ No significant issues found. |
The CSV inputs were opened without an explicit encoding, so on Windows the system ANSI codepage was used and UTF-8-with-BOM files (the default for Excel "Save as CSV (UTF-8)") had \ufeff prepended to the first column name, silently breaking schema detection and the "underscore-prefix means private ID" heuristic. Use `utf-8-sig` everywhere a CSV input file is opened: it transparently strips a leading BOM if present and behaves like `utf-8` otherwise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
832c81c to
519a1e4
Compare
Agent Review SummaryPR: fix(csv): open input files as
|
| Phase | Action | Result |
|---|---|---|
| Code review | Reviewed diff in bulk_update.py and entity_file.py |
✅ Correct fix |
| Completeness check | Audited all open() / io.open() calls in falkordb_bulk_loader/ |
✅ No remaining read-text opens without encoding= |
| Rebase | Branch was 5 commits behind master |
✅ Rebased and force-pushed |
| CI | Monitored all checks after rebase push | ✅ 7/7 checks passing |
Code Analysis
The fix is exactly right: switching from implicit-encoding opens to encoding="utf-8-sig" in all three CSV-reading call sites:
count_entities()inbulk_update.pyprocess_update_csv()inbulk_update.pyEntityFile.__init__()inentity_file.py
The utf-8-sig codec strips a leading UTF-8 BOM (\ufeff) if present and is otherwise identical to utf-8, so there is zero risk of regression for files without a BOM. The root cause (Excel's default "Save as CSV (UTF-8)") is a common real-world issue.
Notes
- No new tests were added for BOM-handling paths. Adding a parametrized unit test that writes a BOM-prefixed CSV and verifies correct column name parsing would strengthen coverage, but is not blocking for this fix.
- The existing test suite passes cleanly on Python 3.10 and 3.11 after the rebase.
Reviewed and rebased by GitHub Copilot agent.
Summary
The CSV input files were opened without an explicit encoding (
io.open(filename, "rt")/open(filename, "rt")), so on Windows the system ANSI codepage was used and UTF-8-with-BOM files (the default for Excel "Save as CSV (UTF-8)") had\ufeffprepended to the first column name. This silently broke:name:TYPEfield was now\ufeffname:TYPE),Label.process_schemaless_header,Changes
Use
encoding="utf-8-sig"everywhere a CSV is opened.utf-8-sigtransparently strips a leading BOM if present and behaves like plainutf-8otherwise.falkordb_bulk_loader/entity_file.py:EntityFile.__init__opensself.infilewithencoding="utf-8-sig".falkordb_bulk_loader/bulk_update.py: same forcount_entitiesandprocess_update_csv.Testing
Lint clean (
flake8,black).Memory / Performance Impact
Negligible;
utf-8-sigonly adds a single optional 3-byte BOM check at file open.Related Issues
From the comprehensive code-review report (BUG-6).