Fix handling of timestamps when ingesting from CSV #16
Conversation
| time_config = schema.time_config | ||
| exprs = [] | ||
| for i, column in enumerate(rel.columns): | ||
| expr = column | ||
| if isinstance(time_config, DatetimeRange) and column == time_config.time_column: | ||
| time_type = rel.types[i] | ||
| if time_type == duckdb.typing.TIMESTAMP and time_config.start.tzinfo is not None: # type: ignore | ||
| expr = f"timezone('{time_config.start.tzinfo.key}', {column}) AS {column}" # type: ignore | ||
| exprs.append(expr) |
There was a problem hiding this comment.
This was the main problem. Now we are applying time zones.
There was a problem hiding this comment.
It seems like we may not need any of the timezone enums we created in time.py since we can use tzinfo directly.
There was a problem hiding this comment.
That might be the case. We might need them if we accept user input definitions as command line arguments or in configuration files.
| table.create(self._engine) | ||
|
|
||
| with self._engine.begin() as conn: | ||
| write_database(rel.to_df(), conn, dst_schema) | ||
| try: | ||
| check_timestamps(conn, table, dst_schema) |
There was a problem hiding this comment.
Previous behavior was incorrect. We need to perform checks on the final table and rollback on error.
|
|
||
| from chronify.exceptions import InvalidTable | ||
| from chronify.models import TableSchema | ||
| from chronify.sqlalchemy.functions import read_database | ||
| from chronify.utils.sql import make_temp_view_name | ||
|
|
||
|
|
||
| def check_timestamps(conn: Connection, table: Table, schema: TableSchema) -> None: |
There was a problem hiding this comment.
The semantics of this are now different. The user calls this function instead of instantiating the class. The function receives the connection instead of the engine and metadata because this needs to happen within a transaction so that erroneous changes can be rolled back.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16 +/- ##
==========================================
- Coverage 94.24% 93.50% -0.75%
==========================================
Files 18 18
Lines 782 831 +49
==========================================
+ Hits 737 777 +40
- Misses 45 54 +9 ☔ View full report in Codecov by Sentry. |
src/chronify/time_series_checker.py
Outdated
| filters = [f"{x} IS NOT NULL" for x in schema.time_config.list_time_columns()] | ||
| def _run_timestamp_checks_on_tmp_table(self, table_name: str) -> None: | ||
| id_cols = ",".join(self._schema.time_array_id_columns) | ||
| filters = [f"{x} IS NOT NULL" for x in self._schema.time_config.list_time_columns()] | ||
| where_clause = "AND ".join(filters) |
There was a problem hiding this comment.
This needs to be " AND ", space in front of and behind.
There was a problem hiding this comment.
Thanks. Fixed.
| time_config = schema.time_config | ||
| exprs = [] | ||
| for i, column in enumerate(rel.columns): | ||
| expr = column | ||
| if isinstance(time_config, DatetimeRange) and column == time_config.time_column: | ||
| time_type = rel.types[i] | ||
| if time_type == duckdb.typing.TIMESTAMP and time_config.start.tzinfo is not None: # type: ignore | ||
| expr = f"timezone('{time_config.start.tzinfo.key}', {column}) AS {column}" # type: ignore | ||
| exprs.append(expr) |
There was a problem hiding this comment.
It seems like we may not need any of the timezone enums we created in time.py since we can use tzinfo directly.
daniel-thom
force-pushed
the
fix/ingest-csv
branch
from
f70ccfb
to
ba28060
Compare
No description provided.