Conversation
skrawcz
left a comment
There was a problem hiding this comment.
AI-generated review: the two inline comments below were produced with AI assistance and should be evaluated accordingly. They are based on local reproduction against commit a0e4f3f7.
| when the write reports no count, e.g. with a custom pandas ``method``). | ||
| :param db_connection: the connection the operation ran on. Optional; when omitted the | ||
| datasource is unknown and ``notes`` says so. | ||
| :param schema: the schema the table was explicitly written to, when the writer had one. |
There was a problem hiding this comment.
AI-generated review comment (validated with a local reproduction): Treating every string containing whitespace as a statement misclassifies valid table names. For example, PandasSqlWriter(table_name="daily revenue", ...) successfully writes that table, but this helper records query="daily revenue", table_name=None, and operation="read"; the OpenLineage parser then reports an error and no output dataset is emitted. Quoted or otherwise unusual identifiers have the same ambiguity. Could the reader/writer pass the operation explicitly (and ideally whether this argument is a table target), rather than inferring both concepts from the string/result shape? Please add a regression test for a table name containing whitespace.
There was a problem hiding this comment.
Good catch. Readers and writers now pass the operation explicitly: get_sql_metadata(..., operation="read"|"write"). PandasSqlWriter(table_name="daily revenue") records operation="write", and in datasource mode (with openlineage-sql installed) emits one output dataset for daily revenue. The whitespace heuristic is gone.
For compatibility, query/table_name keep the 1.0.0 filing, so a written name like USER_SELECTIONS is still filed under query. operation is what marks it as the target table, and sql_datasets() names it as a table. The only new filing is on reads: a statement starting with select/with in any case, after comments, counts as a query. A read string that isn't a plain name, like (select ...), is parsed rather than used as a dataset name. For writes, a plain identifier is the table. Anything else is parsed: a statement contributes the tables it writes, and a string that names no table is the table name. Without the parser (e.g. Windows), such a non-plain name is left out with a note rather than guessed. There are regression tests for daily revenue and SELECTED_ROWS end to end, plus the classification matrix.
Changes and this reply were prepared with AI assistance (Claude).
|
|
||
| return [ | ||
| ( | ||
| part(t.database, t.quote_style, "database"), |
There was a problem hiding this comment.
AI-generated review comment (validated with a local reproduction): For SQLite, schema can name an attached database, but the namespace here always uses source["database"], which is the main file. A query against reporting.orders stored in attached.db is therefore emitted as sqlite://.../main.db + reporting.orders, attributing the dataset to the wrong physical datasource. Since the docs explicitly advertise attached-database naming, could metadata inspection retain the PRAGMA database_list mapping and resolve the schema to the attached file's namespace? Please cover this with a raw SQLite connection that attaches a second file.
There was a problem hiding this comment.
Fixed. For a raw sqlite3 connection, metadata now keeps the PRAGMA database_list mapping as source["attached"] (schema name → absolute file). reporting.orders in an attached file is named sqlite://<attached file> + orders, and main/unqualified tables keep the main file's namespace. Where the mapping can't be known without opening a connection (URL strings, SQLAlchemy engines/connections), a schema-qualified table is left out with a note rather than attributed to the main file. temp is also left out, and an in-memory main database still resolves its attached files.
Covered by a raw-connection test that attaches a second file, both at the sql_datasets() level and end to end through the adapter.
Changes and this reply were prepared with AI assistance (Claude).
|
AI-generated compatibility review comment: One additional concern for existing Could we provide a staged compatibility path instead of changing the default immediately? For example, an adapter option such as I would avoid turning lineage conversion or identity migration into a hard execution error: the SQL operation has already succeeded, and failing the Hamilton node would violate the PR's stated non-interference guarantee. A targeted warning (with strict/error behavior available only as an explicit opt-in, if desired) seems safer. Please also add upgrade tests covering the legacy mode/default and the opt-in datasource mode. |
…eration; SQLite attached files Addresses review on apache#1720: - OpenLineageAdapter gains sql_dataset_identity="legacy" | "datasource". The default stays "legacy": SQL datasets keep the job namespace and bare table name, exactly as before, and need no openlineage-sql. Leaving the option unset emits one FutureWarning per adapter with migration steps; the default changes only in a future major release. create_input_dataset and create_output_dataset keep their original SQL handling and return shapes. - get_sql_metadata takes operation=. PandasSqlWriter passes "write", so a table name such as "daily revenue" is always recorded as a table. The reader recognises lower-case select/with statements. The two-argument form keeps the 1.0.0 classification. - A raw sqlite3 connection records its attached databases, and a table in an attached database is named in that file's namespace. Where the file cannot be known, the table is left out rather than attributed to the main database. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks. The identity change is now staged instead of immediate, as you suggested:
Changes and this reply were prepared with AI assistance (Claude). |
8419ae7 to
6b46405
Compare
Hamilton's built-in SQL loaders/savers (`@load_from.sql`, `@save_to.sql`, `from_.sql`/`to.sql`, and the underlying `PandasSqlReader`/`PandasSqlWriter`) now capture enough datasource context on their own for table-level lineage, without any custom loader or hand-maintained mapping. Motivating use case: a graph reads a query joining `orders` and `customers` from a sales database, aggregates daily revenue in Python, and writes `daily_revenue` to a separate warehouse database. Previously the emitted SQL metadata had no table name for a query read, and the OpenLineage adapter filed writes under the job namespace with a bare table name, so a report written by one job and read by another could not be connected, and same-named tables in different databases were indistinguishable. Users had to write a custom loader and duplicate connection details to get real lineage. Core (`hamilton/io/utils.py`): - `get_sql_metadata()` gains optional keyword `db_connection`/`schema` params and a version-1.1.0 `sql_metadata` shape: `schema`, `operation` (read/write), `source` (dialect, host, port, database, default schema — scalars only, never credentials or live objects), and `notes` explaining an unresolved source. The legacy two-argument call keeps its keys and row-count semantics. - New `get_sql_source()` inspects SQLAlchemy URL strings, Engines and Connections, and raw `sqlite3` connections, read-only: no new connection, no transaction, no session-setting query. In-memory SQLite is deliberately left unidentified rather than given a shared/fabricated identity. Pandas SQL I/O (`hamilton/plugins/pandas_extensions.py`): - `PandasSqlReader`/`PandasSqlWriter` pass their connection and (for the writer) explicit schema into the new metadata; the reader's connection type is widened to `Any` to match what it already accepted in practice (SQLAlchemy Engines/Connections, not just `str | sqlite3.Connection`). OpenLineage integration (`hamilton/plugins/h_openlineage.py`): - New reusable `sql_datasets(sql_metadata, operation=None) -> SqlDatasets` converts Hamilton's SQL metadata into OpenLineage datasets using the `openlineage-sql` parser: every physical table a query reads/writes is named per the OpenLineage naming convention (`postgres://host:port` + `database.schema.table` for PostgreSQL, `sqlite://path` + `table` for SQLite), aliases and CTEs excluded, quoting/case-folding respected. Emits nothing, opens nothing, so a future Airflow provider (or anything else) can call it directly. - `OpenLineageAdapter` routes SQL nodes through it, so dataset identity comes from the datasource rather than the job namespace — a read of a report now resolves to the same identity it was written under. A conversion failure is logged (exception type only, never a message that might quote a connection string) and the run event is still emitted without datasets; the underlying SQL I/O is never affected. Packaging/CI: - `openlineage` extra gains `openlineage-sql` (no Windows wheel; marked `sys_platform != 'win32'` so the base extra still installs there). - `.github/workflows/hamilton-main.yml` adds a disposable `postgres:16` service and a "Test openlineage" step across the Python matrix. Tests: `tests/io/test_utils.py` and `tests/plugins/test_pandas_extensions.py` extended; new `tests/plugins/test_h_openlineage.py` covers dataset naming (SQLite and PostgreSQL, env-gated via `HAMILTON_TEST_POSTGRES_URL`), schema precedence, credential-leak negatives, and that metadata/parser failures never turn a successful SQL read/write into a failed node. A disposable per-test PostgreSQL schema fixture is added to `tests/conftest.py`. Docs: `docs/concepts/materialization.rst` gains a "SQL metadata and lineage" section (fields, supported connections, precedence, failure behavior); `docs/reference/lifecycle-hooks/OpenLineageAdapter.rst` documents dataset naming, the identity change from job-scoped SQL datasets, and the reusable `sql_datasets()` entry point. The `examples/openlineage` example is rewritten around the revenue-reporting story on two local SQLite files, runnable with no external server. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up to the maintainer's review. Each decision below, with its context and the alternatives rejected, is recorded in writeups/adr/: - 2609-01: OpenLineageAdapter(sql_dataset_identity="legacy" | "datasource"). The default stays "legacy", which emits exactly what earlier releases emitted and needs no openlineage-sql. Leaving it unset warns once (FutureWarning) with migration steps. Lineage never fails a node. create_input_dataset/create_output_dataset keep their SQL handling. - 2609-02: SQL metadata keeps the 1.0.0 query/table_name filing, and get_sql_metadata(operation=) carries read/write intent. Reads also recognise select/with statements in any case (a linear scan). - 2609-03: datasource mode resolves what a write names by parsing: a plain name is the table, a statement contributes its written tables, and nothing is guessed without a parse. The job's sql facet comes from the statement actually resolved. - 2609-04: SQLite tables in attached databases are named by their own file. Docs, examples and tests are updated to match. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
6b46405 to
ba453b7
Compare
Related: #1564
Problem
A team uses Hamilton's built-in SQL loaders to read
ordersandcustomers, transforms the data in Python, and uses a built-in SQL saver to write daily revenue into a reporting database. The data operation succeeds, but its metadata does not identify the database/server or reliably qualify the tables involved:@load_from.sql(query_or_table=...)) records the SQL text but no table name at all.Today, answering "which source tables fed this report?" requires writing a custom loader, duplicating connection details, or teaching each downstream integration (e.g. an OpenLineage consumer, an orchestrator) to inspect Hamilton internals.
Use case this PR is built around
A Hamilton graph reads a query joining
sales.public.ordersandsales.public.customerson a source PostgreSQL server, computes daily revenue totals in Python, and writesanalytics.reporting.daily_revenueon a separate warehouse server:No custom loader, no manually-supplied lineage mapping — just the normal
@load_from.sql/@save_to.sqldecorators and the connections the author already has. With this change, Hamilton's OpenLineage adapter reports:examples/openlineageis rewritten around exactly this story (on two local SQLite files, so it runs with no external server) — see its README for the before/after metadata and a runnable walkthrough.What changed
Core metadata (
hamilton/io/utils.py)get_sql_metadata()gains optional keyword-onlydb_connectionandschemaparams. Thesql_metadatadict it returns is now version1.1.0: on top of the existingrows/query/table_name/timestamp, it addsschema(the writer's explicit schema, if any),operation("read"/"write"/Nonefor legacy calls),source(a plain dict ofdialect/host/port/database/default_schema— scalars only, never a connection object or credentials), andnotesexplaining whysourceisNonewhen it is.get_sql_source()inspects SQLAlchemy URL strings,Engines,Connections, and rawsqlite3.Connections, entirely read-only: it never opens a new connection, starts a transaction, or changes session state. In-memory SQLite databases (:memory:,sqlite://,sqlite:///:memory:) are deliberately left unidentified rather than given a shared or fabricated identity.operation="read"|"write"lets callers say what the string is. The pandas reader and writer pass it, so a written table name such asdaily revenueis always the table written.query/table_namekeep the 1.0.0 filing ("SELECT" in text) for writes and for the original two-argument call. The only change is on reads: a statement starting (after comments) withselect/within any case is now filed as a query.sqlite3connection,source["attached"]maps each attached database to its file. It isNonewhere that can't be known without opening a connection.Pandas SQL I/O (
hamilton/plugins/pandas_extensions.py)PandasSqlReader/PandasSqlWriternow pass theirdb_connection, theiroperation, and (for the writer) its explicitschemainto the new metadata call. The reader'sdb_connectiontype annotation is widened fromstr | sqlite3.ConnectiontoAny, since it already accepted SQLAlchemyEngine/Connectionobjects in practice.OpenLineage integration (
hamilton/plugins/h_openlineage.py)sql_datasets(sql_metadata, operation=None) -> SqlDatasetsconverts Hamilton's SQL metadata into OpenLineage datasets, using theopenlineage-sqlparser to resolve every physical table a query reads or writes (aliases and CTEs are excluded). Datasets are named per the OpenLineage naming convention:postgres://{host}:{port}+{database}.{schema}.{table}for PostgreSQL (folding unquoted identifiers, as the server does), andsqlite://{absolute path}+{table}for SQLite. It emits no events and opens no connections, so it's a clean boundary for another integration (e.g. a future Airflow provider) to reuse without going through Hamilton's own event-emitting adapter.OpenLineageAdapter(..., sql_dataset_identity="legacy" | "datasource"). The default,"legacy", keeps today's SQL dataset names exactly (job namespace + bare table name) and needs noopenlineage-sql. Leaving the option unset emits oneFutureWarningper adapter pointing to the migration docs; the default flips only in a future major release.create_input_dataset/create_output_datasetkeep their existing SQL handling and return shapes."datasource", the adapter routes SQL loader/saver nodes throughsql_datasets(), so SQL dataset identity comes from the datasource rather than the job namespace — a later read ofdaily_revenueresolves to the same identity it was written under, regardless of job namespace. Anything that can't be fully identified (unknown source, unsupported dialect, missing schema, parser error, missingopenlineage-sqlinstall) is logged and left out rather than guessed. A conversion failure logs only the exception type (never a message that might quote connection details) and the run event is still emitted without datasets — SQL I/O that already succeeded is never turned into a failed node by a lineage problem.Packaging / CI
openlineageextra gainsopenlineage-sql, markedsys_platform != 'win32'since it publishes no Windows wheel; the base extra (and the OpenLineage client) still installs cleanly there, with SQL table resolution unavailable..github/workflows/hamilton-main.ymladds a disposablepostgres:16service and a "Test openlineage" step to the existing matrix.Tests
tests/io/test_utils.pyandtests/plugins/test_pandas_extensions.pyextended for the new metadata fields, connection forms (URL string,Engine,Connection, rawsqlite3), and credential-leak negatives.tests/plugins/test_h_openlineage.py: dataset naming for SQLite and PostgreSQL (the latter env-gated viaHAMILTON_TEST_POSTGRES_URL, skipped otherwise), schema precedence (SQL-qualified → writerschema=→ connection default), an end-to-end revenue-reporting run with realFileTransport-captured OpenLineage events, and failure-injection tests proving metadata/parser errors never fail a node that already succeeded.tests/conftest.pygains apostgres_schemafixture that creates and drops its own throwaway schema on whatever serverHAMILTON_TEST_POSTGRES_URLnames.Docs
docs/concepts/materialization.rst: new "SQL metadata and lineage" section — field reference, supported connection forms, schema precedence, and what happens when a datasource can't be identified.docs/reference/lifecycle-hooks/OpenLineageAdapter.rst: thesql_dataset_identityoption, the datasource naming table, a "Migrating to the datasource identity" section (what changes for a lineage backend, which datasets stop appearing, and step-by-step migration), and the reusablesql_datasets()entry point.examples/openlineage: rewritten end-to-end around the revenue-reporting use case above, opting intosql_dataset_identity="datasource"; runs locally with no external service viaOpenLineageClient'sFileTransport.Compatibility
OpenLineageAdapterbuilt as before emits the same SQL datasets as the previous release: same namespace, name, and facets. That is pinned in tests againstmain's adapter for query reads (including lower-caseselect/with), table reads, table writes (including names containingSELECTor spaces), and hand-built metadata. The datasource identity is opt-in viasql_dataset_identity="datasource". The one-timeFutureWarningfor the unset default never fails a node, even under-W error. The default flips only in a future major release; the adapter docs describe the migration.openlineage-sqlis needed only for the datasource identity. The default path doesn't import it, so locked environments and Windows keep working unchanged.get_sql_metadata(query_or_table, results)still works and returns the same keys with the same values. New keys (schema,operation,source,notes) are additive, and__version__is1.1.0.PandasSqlReader/PandasSqlWritermetadata keeps the 1.0.0query/table_namefiling, with one exception: a read starting withselect/within any case is now filed as a query (previously a lower-case one was filed as a table name). Legacy-mode OpenLineage names re-derive the 1.0.0 filing, so they are unaffected.db_connectionannotation is widened toAny. It previously rejected SQLAlchemyEngineinputs at graph validation, so this is purely additive.Checklist
AI Disclosure
Codex (GPT-6-Astra) was used to author this PR. The review-driven revisions (staged OpenLineage identity, explicit SQL operation, SQLite attached databases) were made with Claude Code (Claude Opus 5.5).