potiuk commented on PR #70816:
URL: https://github.com/apache/airflow/pull/70816#issuecomment-5149211219
Three separate changes here, and they're at quite different levels of
confidence — worth splitting the discussion.
**The `selected_fields` parsing fix is clearly right.** `list("id,name")`
iterated the string character by character, so the "field names" came out as
`['i', 'd', ',', 'n', ...]`. The new split-and-strip handles the documented
comma-separated form, and `" id, name, "` in the test covers the whitespace and
trailing-empty cases nicely.
**The `get_conn()` -> `connection` fix is right and is probably the most
consequential change here — but nothing tests it.**
`get_openlineage_database_info` documents its parameter as *"Airflow connection
to reduce calls of `get_connection` method"*, and `PostgresHook`'s
implementation reaches for `connection.extra_dejson` and
`get_openlineage_authority_part(connection)`. A DBAPI connection has neither,
so against a real hook this raised `AttributeError` and the facets never got
emitted.
The trouble is the test can't see the difference:
```python
mock_postgres_hook.get_openlineage_database_info.return_value = db_info
```
A `MagicMock` accepts whatever it is handed, so that assertion passes
identically with `get_conn()` or `connection`. Could you pin the argument, so a
regression here is caught?
```python
mock_postgres_hook.get_openlineage_database_info.assert_called_once_with(mock_postgres_hook.connection)
```
**The third change needs a decision I don't think should be made in
passing.** Copying the input's schema facet onto the output dataset does make
Marquez render the field nodes, but `get_facets_from_bq_table_for_given_fields`
builds those fields as:
```python
SchemaDatasetFacetFields(name=schema_field.name,
type=schema_field.field_type, ...)
```
`field_type` is the **BigQuery** type — `STRING`, `INTEGER`, `NUMERIC`,
`TIMESTAMP`. Attaching that to the output dataset advertises BigQuery types for
a Postgres, MSSQL or MySQL table. The field names carry over correctly, but the
types describe the wrong system, and any consumer other than Marquez reads them
at face value.
So the change buys rendering at the cost of type metadata that is wrong.
Emitting the fields with names only would be honest and should still give
Marquez enough to build the nodes — but whether that is the right call, or
whether approximating with source types is accepted practice, is an OpenLineage
semantics question rather than a detail of this PR.
@mobuchowski @kacpermuda — could you weigh in on that last point?
Specifically: is it acceptable for a transfer operator's output dataset to
carry the source system's schema facet verbatim, types included, or should the
types be omitted or mapped when the output is a different backend?
The first two changes look good to me and I'd be happy to merge them once
the argument assertion is in.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
