Repository navigation
Replace SQLAlchemy with ibis-framework for all database operations - #66
Draft
daniel-thom wants to merge 83 commits into
Draft
daniel-thom wants to merge 83 commits into
daniel-thom wants to merge 83 commits into
Conversation
Replace the SQLAlchemy ORM with ibis-framework to provide a cleaner multi-backend abstraction for DuckDB, SQLite, and Spark. This is a clean API break: engine->backend, Connection params removed, ibis expressions replace SQLAlchemy select/join chains. Key changes: - New src/chronify/ibis/ module with IbisBackend ABC and DuckDB/SQLite/Spark implementations - Remove SQLAlchemy, pyhive vendor code, Hive support, and related dependencies - Migrate all source modules (store, mappers, checker, converters) to ibis API - Migrate all tests to use backend fixtures instead of engine fixtures - Add ibis-framework[duckdb,sqlite] dependency, pyspark >= 4.0 for spark extra Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates Chronify’s database layer from SQLAlchemy/Hive-specific code to an ibis-based backend abstraction, aiming to support multiple execution engines (DuckDB, SQLite, Spark) with a unified API.
Changes:
- Introduces
src/chronify/ibis/with backend implementations and I/O helpers (read_query,write_table, Parquet helpers). - Refactors mappers/checkers/time zone utilities and tests to use
IbisBackendinstead ofsqlalchemy.Engine/MetaData. - Removes SQLAlchemy + Hive/pyhive vendor code and updates project dependencies accordingly.
Reviewed changes
Copilot reviewed 53 out of 56 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_time_zone_localizer.py | Switches timezone localization tests from SQLAlchemy engine fixtures to IbisBackend fixtures. |
| tests/test_time_zone_converter.py | Switches timezone conversion tests to the ibis backend API. |
| tests/test_time_series_checker.py | Updates timestamp-checker tests to call check_timestamps(backend, table_name, schema). |
| tests/test_models.py | Updates dtype tests to use ibis datatypes (ibis.expr.datatypes). |
| tests/test_mapper_representative_time_to_datetime.py | Refactors representative-time mapper tests to use read_query/write_table. |
| tests/test_mapper_index_time_to_datetime.py | Refactors index-time mapper tests to use ibis backend querying. |
| tests/test_mapper_datetime_to_datetime.py | Updates datetime-to-datetime mapping tests and write-table duplicate-config behavior expectations. |
| tests/test_mapper_column_representative_to_datetime.py | Updates Store fixture + mapping assertions to use ibis querying instead of SQLAlchemy. |
| tests/test_csv_parser.py | Updates parser tests to construct Store(backend=...). |
| tests/test_checker_representative_time.py | Updates checker tests to use ibis backend and new check_timestamps signature. |
| tests/conftest.py | Replaces SQLAlchemy engine fixtures with ibis backend fixtures (iter_backends, make_backend). |
| src/chronify/utils/sqlalchemy_view.py | Removes SQLAlchemy view DDL utilities. |
| src/chronify/utils/sqlalchemy_table.py | Removes SQLAlchemy “CREATE TABLE AS SELECT” utilities and SQLite timestamp workaround. |
| src/chronify/time_zone_localizer.py | Refactors timezone localization to backend-driven querying + mapping. |
| src/chronify/time_zone_converter.py | Refactors timezone conversion to backend-driven querying + mapping. |
| src/chronify/time_series_mapper.py | Updates public mapping entrypoint to accept IbisBackend. |
| src/chronify/time_series_mapper_representative.py | Refactors mapper to fetch distinct time zones via ibis expressions. |
| src/chronify/time_series_mapper_index_time.py | Refactors index-time mapper to fetch distinct time zones via ibis expressions. |
| src/chronify/time_series_mapper_datetime.py | Refactors datetime mapper base wiring to use IbisBackend. |
| src/chronify/time_series_mapper_column_representative_to_datetime.py | Replaces SQLAlchemy-based intermediate table creation with ibis table creation + joins. |
| src/chronify/time_series_mapper_base.py | Reimplements mapping application as ibis join/aggregate + Parquet output support. |
| src/chronify/time_series_checker.py | Refactors timestamp checks to use backend SQL execution and ibis selects. |
| src/chronify/sqlalchemy/functions.py | Removes SQLAlchemy read/write optimization helpers. |
| src/chronify/schema_manager.py | Refactors schema persistence to a backend-managed table and DataFrame inserts. |
| src/chronify/models.py | Replaces SQLAlchemy dtype handling with ibis dtype handling and new conversion helpers. |
| src/chronify/ibis/base.py | Adds IbisBackend ABC and common helpers (raw SQL exec, “transaction” cleanup). |
| src/chronify/ibis/init.py | Exposes backends and make_backend factory. |
| src/chronify/ibis/duckdb_backend.py | Implements DuckDB backend wrapper. |
| src/chronify/ibis/sqlite_backend.py | Implements SQLite backend wrapper (including insert + Parquet handling). |
| src/chronify/ibis/spark_backend.py | Implements Spark backend wrapper and Spark-specific datetime preparation. |
| src/chronify/ibis/functions.py | Adds backend-agnostic read/write utilities (table/query, parquet, datetime conversions). |
| src/chronify/ibis/types.py | Adds DuckDB/ibis type conversion and dataframe schema inference helpers. |
| src/chronify/hive_functions.py | Removes Hive materialized-view workaround. |
| src/chronify/csv_io.py | Updates CSV dtype mapping to use ibis/duckdb conversion. |
| src/chronify/init.py | Removes pyhive/TCLIService module injection. |
| src/chronify/_vendor/kyuubi/* | Removes vendored pyhive/kyuubi code. |
| pyproject.toml | Drops SQLAlchemy/pyhive deps and adds ibis + pyspark extra. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix create_view_from_parquet to return ObjectType so callers drop the correct object type (SQLite creates a table, not a view) - Handle directory paths in DuckDB create_view_from_parquet for partitioned parquet datasets - Add unique index on schemas table name column to prevent duplicates - Escape single quotes in schema remove_schema to prevent SQL injection - Add dispose() teardown to test fixtures to prevent resource leaks - Use explicit column ordering in DuckDB insert to prevent column mismatch when DataFrame column order differs from table - Clean up schema on failed ingestion rollback in all Store methods Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 56 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
src/chronify/time_series_mapper_column_representative_to_datetime.py:1
- This intermediate mapping table previously used replace semantics, but now
if_exists=\"fail\"will error if a prior run crashed (or a previous test left the table behind). That can make retries flaky. Consider switching toif_exists=\"replace\"or explicitly dropping the mapping table before creating it so the workflow is robust to partial failures.
tests/conftest.py:1 - This fixture still uses the name
iter_stores_by_engineeven though it now iterates over backends. Renaming it (and any references) to something likeiter_stores_by_backendwould make the updated abstraction clearer and reduce confusion for future contributors.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
daniel-thom
force-pushed
the
feat/replace-sqlalchemy-with-ibis
branch
from
April 10, 2026 19:53
e453b76 to
177ca7e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 58 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
===========================================
+ Coverage 52.47% 90.94% +38.47%
===========================================
Files 59 37 -22
Lines 12952 3215 -9737
===========================================
- Hits 6797 2924 -3873
+ Misses 6155 291 -5864 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
daniel-thom
commented
Apr 12, 2026
- Allow injecting an existing ibis connection into DuckDB/SQLite backends,
mirroring SparkBackend(session=...); external connections are not
disposed by the backend.
- Remove IbisBackend.reconnect; replace with backend-native backup():
DuckDB uses disconnect+copy+reconnect internally, SQLite uses the
sqlite3 online backup API, Spark raises InvalidOperation.
- Validate insert column sets on DuckDB/SQLite/Spark and raise
InvalidParameter on mismatch instead of silent reindex.
- DuckDB write_parquet uses COPY (FORMAT PARQUET) for the unpartitioned
path instead of materializing to pandas.
- SparkBackend.dispose always disconnects the ibis connection and stops
the session only when owned; factor temp-view handling into a helper.
- Collapse duplicated _write_to_{duckdb,sqlite,spark} into a single
_apply_if_exists helper.
- ObjectType is a StrEnum; make_backend raises InvalidParameter for
unknown names; schema_manager imports moved to top.
- Restore docstrings on convert/localize time zone methods and
read_raw_query.
- Relax pyspark pin to >=4.0,<5.
- CI: install package editable and pass --cov=chronify so Codecov can
map coverage paths back to repo sources.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
scratch_dir was removed along with the Hive backend, but its purpose (#51) still applies: on a multi-node cluster, temporary data must live on a shared filesystem. On Spark, chronify's tables, including intermediate mapping tables, are managed tables under spark.sql.warehouse.dir, which defaults to ./spark-warehouse. SparkBackend(warehouse_dir=...) sets it on the session the backend creates. Because the location is fixed once a session starts, combining it with session= raises ConflictingInputsError, and an already-running session with a different warehouse raises InvalidParameter instead of being silently reused. Document the setting in the Spark how-to. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
apply_mapping wrote its mapping table under a fixed name such as 'mapping_table' and dropped any table with that name on failure, so a user table with the same name could be dropped. The Parquet timestamp check view was named after the destination, which forced a TableAlreadyExists error even though Parquet output never touches that table. All temporary tables and views now get random names from make_temp_view_name, which takes an optional prefix. Cleanup only drops objects the call created. Parquet output with check_mapped_timestamps now succeeds when a table with the destination name exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Spark's default table format does not support DELETE, so delete_rows rewrites the table with the rows to keep. The filter NOT (col = :p) evaluates to NULL for rows whose key is NULL, so those rows were dropped too. Use coalesce(NOT (...), true) so they are kept, as DELETE would. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sqlite3 binds numpy integers as 8-byte BLOBs. Values from pandas nullable Int64/Int32 columns iterate as numpy integers, so SQLite stored them as BLOBs and filters, joins and delete_rows could not match them. _adapt_value now converts numpy numbers and bools to Python scalars. Store.delete_rows also converts numpy key values, such as those from df["id"].unique(): DuckDB raised on them and SQLite matched nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PySpark renders TIMESTAMP values as naive wall-clock times in the session time zone, and Ibis labels them with the column's time zone. apply_schema_types labels tz-aware columns as UTC, so reading them through Store.read_table(...).execute() or to_pyarrow() under a non-UTC session (for example America/Denver, or an unset session time zone, which defaults to the JVM's local zone) shifted every value by the UTC offset. Subclass the Ibis PySpark backend so execute() pins the session to UTC when the result has a tz-aware timestamp column. Results with only tz-naive timestamps keep the caller's session time zone, which callers such as dsgrid rely on when they write tz-naive data under a non-UTC session (custom_time_zone) and map it with chronify. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the timestamp check failed, create_view_from_parquet dropped the view through the backend, so the schema registered for it stayed in the schemas table. A retry under the same name then failed with "A schema with name=... is already registered". Drop through the Store methods, which also remove the schema, as main did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Spark converts tz-naive timestamps through spark.sql.session.timeZone when it writes and reads them, so tz-naive data ingested under a non-UTC session fails the timestamp check with a mismatch that doesn't say why. When the check fails on Spark for a tz-naive schema and the session isn't UTC, add a note naming the session time zone and the fix. Add SparkBackend.session_time_zone for the checker to use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ad code - DuckDBBackend.execute runs ibis's pre-execute hooks so memtables are registered before the compiled SQL runs on the raw connection. - DuckDBBackend.backup copies through the open connection with COPY FROM DATABASE, so existing table expressions stay valid. It raises inside a transaction. - SparkBackend removes the lowercased managed-table directory, which is where Spark stores mixed-case table names. - Share _quote_identifier and _validate_insert_columns from ibis.base. - Remove unused type-conversion helpers and an unreachable guard. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SQLiteBackend.create_table and insert executed ibis expressions into pandas and re-inserted the rows from Python. create_table now creates the table from the expression's schema (CREATE TABLE AS SELECT loses the declared types) and insert runs INSERT INTO ... SELECT. A 1M-row create_table from an expression drops from about 5 s to 0.14 s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-with-ibis Both branches fixed overlapping review findings. Resolutions: - SQLite timestamps: keep ibis2's migrate-to-ISO-on-load (defd562) and drop 461aad1's legacy canonical storage format and apply_schema_types override. Keep 461aad1's delete_rows value adaptation and execute_sql_to_df params support. - SQLite create_table/insert: keep ibis2's in-database path (7bdd56d). - Spark: keep ibis2's session-tz model (tz-naive data stays in the caller's session tz; UTC pinned only when materializing tz-aware results) and drop 461aad1's UTC pins around writes. Remove the read_query UTC pin, which shifted tz-naive data under a non-UTC session; tz-naive ingest under a non-UTC session now passes checks. Bind naive datetime SQL parameters in the session tz. - DuckDB execute: keep ibis2's _run_pre_execute_hooks. - create_view_from_parquet: keep 461aad1's register-after-checks. - map_time overwrite check: keep ibis2's check in apply_mapping. - write_query_to_parquet: keep ibis2's removal of the name parameter; 461aad1's wiring had no effect on any backend's output. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SQLite inserts adapted every cell in Python. Adapt whole columns instead: datetimes via np.datetime_as_string (same text as isoformat), scalars via astype(object). Other dtypes keep the per-cell path. Spark's execute_sql_to_df dropped empty params; check `is not None` like the other backends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- DuckDB: write partitioned Parquet with a direct COPY; ibis renders partition_by as a Python list repr that DuckDB rejects. - Spark: dispose() no longer stops a caller-provided session. - Pivoted ingest normalizes timestamps before wrapping data in a memtable. - list_tables hides all ibis memtables (pandas, pyarrow, ...). - SQLite: materialize expressions containing memtables on insert; keep in-database ibis expressions in SQL on write_table. - Share delete_rows across DuckDB/SQLite and reject empty values. - Remove duplicate Spark LOCATION_ALREADY_EXISTS retry in SchemaManager. - Factor ingest transaction/cleanup into Store._run_ingest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ibis-pyspark creates views with CREATE TEMPORARY VIEW, but their schemas were persisted to the schemas table. In a later session the view was gone while its schema row remained, blocking re-creation of the view. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On SQLite, casting a timestamp compiles to STRFTIME, which reformats the stored ISO text, so a join between a cast and an uncast timestamp column matched no rows and the mapping silently produced an empty table. Format both sides the same way before comparing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The migration rebuilt every table with a DATETIME column, including tables chronify does not manage, which dropped their constraints and indexes and mangled timestamps with UTC offsets. Only migrate registered time columns, and detect them with a single query so opening a current store stays cheap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Guard intermediate-table drops in mapper finally blocks so a cleanup failure no longer replaces the original mapping error. - Pass pandas the real sqlite3 connection inside a transaction instead of the no-commit proxy, which pandas does not recognize. - Reject a database path for the Spark backend instead of silently ignoring it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Replace the hand-written Ibis-to-DuckDB type map in read_csv with ibis' DuckDBType.to_string, which handles types the map rejected (Decimal, UInt32, Time, timestamp precision). - Define dispose() once in IbisBackend, honoring _owns_connection. - Drop a redundant datetime branch in the SQLite value adapter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ibis read_parquet registers a session-only temp view, so persistent views built on it broke after the connection closed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The custom partitioned path compiled and ran the SQL directly, skipping memtable registration. ibis forwards kwargs to DataFrameWriter.save, which accepts partitionBy, so the custom path is unnecessary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace calls to the private _run_pre_execute_hooks with to_pyarrow() for execute and to_parquet(partition_by=...) for partitioned writes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ibis read_parquet left an ibis_read_parquet_* temp view behind that showed up in list_tables(). Create the view with CREATE VIEW over parquet.`path` instead, matching DuckDB and main, and persist its schema. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Spark skipped the empty-table drop to avoid a full count, leaving an empty registered table behind. Check emptiness with limit(1) instead, as main did, so all backends behave the same. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failed COMMIT (e.g. SQLite database is locked) left the transaction open, so the next autocommitting statement committed the partial writes of the block that had reported failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Casting and comparing SQLite timestamps across column types goes through STRFTIME %f, so keys differing only below 1 ms compare equal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
map_table_time_config and the time zone conversion/localization methods created the destination table and then registered its schema separately. If add_schema failed, the table remained without a schema and every retry raised TableAlreadyExists. Run both through _run_ingest so a failure rolls back or cleans up the table. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace copies of the guarded has_table/drop_table/log cleanup with the existing helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflict resolutions: - ci.yml: keep the local PySpark install; drop main's Spark Thrift server setup, which only the SQLAlchemy/Hive path used. - pyproject.toml: take main's version, Python range, and duckdb >= 1.5; drop duckdb_engine. - csv_io.py, models.py: keep this branch; main's changes there updated SQLAlchemy type mapping that this branch removed. Drop the now-unused chronify/duckdb/types.py. Changes required by duckdb >= 1.5: - Require ibis-framework >= 12.0 (older ibis fails on duckdb 1.5) and cap duckdb < 2 (ibis 12 calls fetch_arrow_table, removed in duckdb 2.0). - Replace duckdb.typing.TIMESTAMP in csv_io.py with duckdb.sqltype. - DuckDB 1.5 parses DESCRIBE "schemas" (also "tables", "databases") as a SHOW command, even when quoted. Qualify table lookups and inserts with the current schema, and rename chronify's schemas table to chronify_schemas, migrating legacy databases on open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ibis 12's pyspark extra requires pyspark < 4.1, so pyspark 4.1.1 could not be installed. pandas-stubs was unpinned and resolved to 3.x stubs while pandas is < 3. Remove type: ignore comments made unnecessary by newer stubs, cast the DuckDB Arrow-to-pandas result, and work around pandas-stubs rejecting None as the Series.where replacement. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace the SQLAlchemy ORM with ibis-framework to provide a cleaner multi-backend abstraction for DuckDB, SQLite, and Spark. This is a clean API break: engine->backend, Connection params removed, ibis expressions replace SQLAlchemy select/join chains.
Key changes: