Skip to content

test: pin warehouse definition binding as a shared fixture - #437

Draft
jat255 wants to merge 2 commits into
jat255/8kf5-catalog-merge-error-classesfrom
jat255/5hfw-definition-binding-fixture
Draft

jat255 wants to merge 2 commits into
jat255/8kf5-catalog-merge-error-classesfrom
jat255/5hfw-definition-binding-fixture

Conversation

@jat255

@jat255 jat255 commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

This PR adds a shared fixture, definition-binding.json. It pins how both packages bind a governed definition to the column names a warehouse catalog reports, and which error each package raises when it cannot. Stacked on #436.

Python behaviour change. Since py-v0.1.0b1, COLUMNS(pattern) and COLUMNS(*) dropped an authored column that the warehouse relation does not have. They now refuse it, as a selection written out by name already did, and as R always has (_compile.py). This needs a line in the next py-v* release notes. Python also now checks a dictionary with several faults in the same order as R, so both packages report the same fault.

R changes

R's binding results do not change. Its three binding refusals now have condition classes, listed in NEWS: a metric that mixes row and aggregate grain, a table that matches no relation, and a column the relation lacks. The strict field in the catalog's definition bindings is gone from definition-compile.R. It was always TRUE, so nothing behaves differently. One test in test-definition-compile.R is now a fixture case. The catalog-merge runner's setup code moved into helper-catalog-rows.R, so both runners share it.

Agent-written details
  • kata: 5hfw. Follow-up kata yhfp: a COLUMNS(...) selection that matches no column compiles to empty SQL in both packages, with or without bindings. That bug is older than this PR and is out of scope here.
  • Testing: Python ruff, pyrefly check src tests and the full pytest run pass. The R devtools::test() run passes. The full R CMD check was not run. Each fixture case was checked to fail against code with the behaviour it pins removed, including the old Python selection rule and the old Python check order.
  • Not covered: the "data source does not expose" refusal, used when there is no catalog, has no class in either package. Every fixture case runs the catalog merge, so the fixture does not reach it. Python users can only catch the new classes through private modules, the same as fix: give each catalog merge error its own class in both packages #436's catalog classes.

@jat255 jat255 added r Affects the R implementation py Affects the Python implementation tests Related to testing or the test suite needs-manual-review Agent-created work that needs a human review labels Oct 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Preview root: https://posit-dev.github.io/commons/pr-437/

Python site preview: https://posit-dev.github.io/commons/pr-437/py/

Built from the latest commit on this branch. The R links in it point at the published R site, which no pull request rebuilds.

@jat255
jat255 force-pushed the jat255/8kf5-catalog-merge-error-classes branch from 2528d72 to 2e33b0e Compare October 21, 2026 22:03
@jat255
jat255 force-pushed the jat255/5hfw-definition-binding-fixture branch from 8fca4c4 to 95aed4e Compare October 21, 2026 22:03
Definition binding was specified only in the Python suite, though both
packages implement it. definition-binding.json now pins it: authored
columns lowered with the warehouse's spelling on Snowflake and Databricks,
marker reservation through a column and through a COLUMNS(...) selection,
and the two refusals, as slugs mapped to a class in each runner.

A COLUMNS(...) selection from a pattern or from * now refuses a column the
relation lacks, as one written out by name already did. Python dropped such
a column instead, so the two packages disagreed on a dictionary documenting
a column the warehouse no longer has. R's always-true strict flag goes with
it, since no caller binds any other way.

Both runners share the catalog-merge hydration, moved into a test helper.
A dictionary with several faults got a different refusal from each
package: Python checked every table's match up front and metric grain
last, while R checks grain first and then each table in authored order,
its match before its columns. Python now follows that order, its grain
refusal gets a class in both packages, and three definition-binding cases
pin the order.

The definition error classes are re-exported from commons._definitions,
as the catalog ones are from commons._catalog, and both runners name the
case when a refusal is missing or of the wrong class.
@jat255
jat255 force-pushed the jat255/8kf5-catalog-merge-error-classes branch from 2e33b0e to 97bc0e6 Compare October 22, 2026 02:46
@jat255
jat255 force-pushed the jat255/5hfw-definition-binding-fixture branch from 95aed4e to 073aa85 Compare October 22, 2026 02:46

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-manual-review Agent-created work that needs a human review py Affects the Python implementation r Affects the R implementation tests Related to testing or the test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant