-
Notifications
You must be signed in to change notification settings - Fork 2
perf: cast dates with the format inferred by csv-detective #470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
07a98de
c52e278
000ce41
6fc831e
43b3100
bb5d991
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ dependencies = [ | |
| "aiohttp>=3.10.3", | ||
| "asyncpg>=0.29.0", | ||
| "coloredlogs>=15.0.1", | ||
| "csv-detective==0.12.0", | ||
| "csv-detective", | ||
| "dateparser>=1.1.7", | ||
| "humanfriendly>=10.0", | ||
| "json-stream>=2.3.3", | ||
|
|
@@ -94,6 +94,11 @@ local_scheme = "no-local-version" | |
| [tool.uv] | ||
| constraint-dependencies = ["urllib3>=2.7.0"] | ||
|
|
||
| # `cast()` only takes the inferred date format from that branch on. Pin it back to a released | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer not to add that kind of shim to the version control which decreases readability, maintainability and not as explicit ad the version in pyproject.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand, this PR require the new version of csv-detective and cannot be merge without it… This comment is just here to remind us to change this, no?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would not merge this branch before there is a version of csv-detective released for that! |
||
| # version once datagouv/csv-detective#245 is merged and published. | ||
| [tool.uv.sources] | ||
| csv-detective = { git = "https://github.com/datagouv/csv-detective", branch = "improve_date_parsing" } | ||
|
|
||
| [tool.uv.build-backend] | ||
| module-name = "udata_hydra" | ||
| module-root = "" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,6 +81,31 @@ async def test_csv_to_db_complex_type_casting(db, line_expected, clean_db, fake_ | |
| assert dict(res[0]) == {k: v for k, v in zip(cols, expected)} | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "values_expected", | ||
| ( | ||
| # a value no other value of the column disambiguates is read day-first | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about the date format YYYY/MM/DD? Is it supported by csv-detective? In any case, should we test it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this is supposed to be supported ! As well as YYYY/DD/MM There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree we should test it |
||
| (["05/03/2022"], [date(2022, 3, 5)]), | ||
| # a single day-only value settles the format of the whole column... | ||
| (["03/04/2022", "25/04/2022"], [date(2022, 4, 3), date(2022, 4, 25)]), | ||
| # ...and the same value is read the other way around in a month-first column | ||
| (["03/04/2022", "04/25/2022"], [date(2022, 3, 4), date(2022, 4, 25)]), | ||
| ), | ||
| ) | ||
| async def test_csv_to_db_ambiguous_date_column(db, values_expected, clean_db, fake_check): | ||
| check = await fake_check() | ||
| values, expected = values_expected | ||
| rows = "\n".join(f"{index};{value}" for index, value in enumerate(values, start=1)) | ||
| with NamedTemporaryFile() as fp: | ||
| fp.write(f"int;date\n{rows}".encode("utf-8")) | ||
| fp.seek(0) | ||
| file = Csv(file_name=os.path.basename(fp.name), resource_id=RESOURCE_ID) | ||
| await file.inspect() | ||
| table = await file.to_db(check=check) | ||
| res = await db.fetch(f'SELECT date FROM "{table.table_name}" ORDER BY __id') | ||
| assert [row["date"] for row in res] == expected | ||
|
|
||
|
|
||
| async def test_basic_sql_injection(db, clean_db, fake_check): | ||
| check = await fake_check() | ||
| # tries to execute | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not unpin the csv detective version in pyproject, since it's a base component of hydra we need to track and pin the version explicitly by bumping it manually. The manual version change in the pyproject file is an established process in the repo and a good way to protect from human errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be reverted as soon as the new version is released (this hydra branch require the new csv-detective of datagouv/csv-detective#245)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not merge this branch before there is a version of csv-detective released for that!