feat(tables): add constant-per-key configuration support - #186
feat(tables): add constant-per-key configuration support#186hotdata-automation[bot] wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| ### Changed | ||
|
|
||
| - feat(tables): add constant-per-key configuration support |
There was a problem hiding this comment.
This PR removes public SDK surface, but the ### Removed section above records none of it.
Removed by this diff:
ConnectionsApi.purge_connection_cache(hotdata/api/connections_api.py)ConnectionsApi.purge_table_cache(same file)CreateConnectionRequest.skip_discovery(hotdata/models/create_connection_request.py)
Failure scenario: a caller on 0.10.0 calls connections_api.purge_table_cache(conn, schema, table), upgrades, and gets AttributeError: 'ConnectionsApi' object has no attribute 'purge_table_cache'. A caller passing skip_discovery=True to CreateConnectionRequest gets a pydantic validation error. Neither caller has a release note to read.
The ### Removed block already documents three other breaking removals in this same cycle, so the convention is established. Add a **Breaking:** entry covering these three.
| """ # noqa: E501 | ||
| columns: Optional[List[ColumnInfo]] = None | ||
| connection: StrictStr | ||
| constant_per_key: List[StrictStr] = Field(description="Columns the table declares constant for a given key: for every row, any other row sharing its key holds the same value of these columns. Declaring this lets a keyed mutation narrow its search for prior versions to the values the upload carries. Empty when none is declared, which is the unrestricted search. Unlike `partition_by` and `sorted_by` this is NOT fixed at creation — it changes only which files a mutation opens, never how rows are written — so read it here rather than assuming a declaration took effect.") |
There was a problem hiding this comment.
nit: constant_per_key is declared required on TableInfo, so any code that constructs a TableInfo directly must now supply it. (not blocking)
CHANGELOG.md:65-70 documented the identical change for partition_by and sorted_by in 0.9.0, calling out that "code that constructs TableInfo directly (test mocks, fixtures) must now supply them". Add the same note under ### Changed for this field.
There was a problem hiding this comment.
The full diff was omitted from the review prompt (5181 lines). I fetched it with gh pr diff 186 and read it in full, plus these files from the checkout: CHANGELOG.md, pyproject.toml, README.md, .github/workflows/integration-tests.yml, .github/workflows/check-release.yml, tests/integration/test_connections_read.py, test/test_databases_api.py, hotdata/api/connections_api.py, hotdata/models/table_info.py.
The generated code itself is correct. Serializers, response maps, __properties lists, and from_dict/to_dict for the new models all match the openapi-generator pattern used elsewhere in the repo. The problems are in the release metadata and one stale hand-written test.
Review
Blocking Issues
CHANGELOG.md:24 — three breaking removals are undocumented.
This PR deletes public SDK surface:
ConnectionsApi.purge_connection_cache(hotdata/api/connections_api.py)ConnectionsApi.purge_table_cache(hotdata/api/connections_api.py)CreateConnectionRequest.skip_discovery(hotdata/models/create_connection_request.py)
The Removed block in [Unreleased] records none of them. That block already documents three other breaking removals from this same cycle, so the convention is established and this is an omission.
A caller on 0.10.0 who upgrades gets AttributeError: 'ConnectionsApi' object has no attribute 'purge_table_cache' with no release note explaining it.
The changelog line this PR adds is seeded automatically from the commit title, so it describes only the feature. The removals are collateral spec drift and need a manual entry.
Action Required
Add a **Breaking:** entry under Removed in [Unreleased] naming purge_connection_cache, purge_table_cache, and CreateConnectionRequest.skip_discovery.
Non-blocking
nit: tests/integration/test_connections_read.py:32 calls purge_connection_cache, which this PR removes. The test carries @pytest.mark.skip, so pytest does not run it and CI stays green. Whoever removes the skip — the reason says "Re-enable once the seeded connection is restored" — hits AttributeError. mypy also covers tests/ per pyproject.toml:47-51, though no CI job runs it. Delete lines 31-32 and drop "and cache purge" from the module docstring at line 4. (not blocking)
nit: TableInfo.constant_per_key is required. See the inline comment on hotdata/models/table_info.py. (not blocking)
CI note: at review time only Integration Tests / scenario-parity had reported success. unit, integration, and the release-metadata check were still queued or running, so I make no claim about them.
Auto-generated from the updated HotData OpenAPI spec.
Source: https://github.com/hotdata-dev/www/pull/409