Skip to content

Commit dc3e164

Browse files
rustyconoverclaude
andcommitted
Fix vgi.v3 regressions: tolerant overrides, derived nullability, ATTACH checks
Workers moving to vgi.v3 hit failures the SDK should have absorbed or caught in-process. This addresses each one. - Signature-tolerant dispatch (vgi/catalog/_dispatch.py): the worker always passes client_capabilities= to catalog_attach, options= to schema_create, aliases/security/comment/tags/column_comments to view_create, and so on, so a 2.x override with explicit parameters raised TypeError on every ATTACH. call_catalog() inspects the override and passes only what it accepts, mapping column_path to column_name for top-level paths. A dropped argument emits a DeprecationWarning plus one log line; a meaningful value (barrier view, schema options, nested path, constraint definitions) raises instead of being silently ignored. - Nullability is derived from records (served_table_schema): a non-nullable field no NOT_NULL/PRIMARY_KEY record names is served nullable, a field a record names is served non-nullable, recursing into ordinary structs and skipping is_row_id, extension and logical-type storage. Applied on every serve path (tables list, table_get, catalog_contents, time travel). - Constrained writable tables: ReadOnlyCatalogInterface advertises vgi.constraint.enforce.v1 when a table has records and write functions, so they are no longer stripped to read-only. If enforcement is not negotiated the worker still serves read-only but logs a warning saying why. - Declared indexes: vgi.index.definition.read.v1 is advertised automatically; the example fixture's hand-written override is gone. index_definition() and TypedValueBatch.from_values() build definitions without hand-assembling records; Index.options defaults to the definition's options. - validate_constraint_records() mirrors DecodeNativeConstraints: owner table and schema, duplicate ids, case-insensitive duplicate names, one PRIMARY KEY, path resolution (nested only for NOT_NULL, never is_row_id), FK same schema/MATCH SIMPLE/NO_ACTION|RESTRICT, CHECK profile/context/grammar. validate_foreign_key_targets() checks referenced tables and keys across the catalog (registry build, tables list, catalog_contents). - CHECK and generated-column grammars are checked with DuckDB's own parser (json_serialize_sql) when haybarn/duckdb is installed, mirroring ParseNumericCheck and the generated Scalar walk. validate_generated_columns() applies VIRTUAL/nullable/type/owner/context rules; legacy generated_expression metadata is translated to the canonical record. - Client: DEFAULT_CLIENT_CAPABILITIES (what the Python client decodes) is the default catalog_attach offer; the CLI reuses it. - Public helpers: catalog_capabilities_for(), CatalogCapabilities.union(), SqlExpression.scalar(). - Legacy API: TableInfo(not_null_constraints=...) and SqlExpression("x") raise TypeErrors naming the replacement; Table's legacy fields type-check only as (); ForeignKeyDef/ForeignKeyInfo are @deprecated. Constrained CREATE TABLE stays refused (no SDK catalog can enforce it) with a message naming what is unsupported. - Cross-SDK example test expects ENFORCED + ALL_ROWS records. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 7c14550 commit dc3e164

24 files changed

Lines changed: 1976 additions & 196 deletions

‎docs/catalog-interface.md‎

Lines changed: 97 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -526,7 +526,7 @@ class UsersFunction(TableFunctionGenerator):
526526
"""Generate user data."""
527527

528528
FIXED_SCHEMA: ClassVar[pa.Schema] = pa.schema([
529-
pa.field("id", pa.int64(), nullable=False), # NOT NULL: see constraints below
529+
pa.field("id", pa.int64()), # served NOT NULL: the PRIMARY_KEY record below names it
530530
("name", pa.string()),
531531
("active", pa.bool_()),
532532
])
@@ -600,7 +600,7 @@ if __name__ == "__main__":
600600
| Feature | Description |
601601
|---------|-------------|
602602
| **No schema duplication** | Function-backed tables derive schema automatically |
603-
| **Constraint validation** | Constraint paths, trust and Arrow nullability validated at definition time |
603+
| **Constraint validation** | DuckDB's ATTACH rules for records and generated columns checked at definition time |
604604
| **Automatic scan handling** | Function-backed tables don't need `table_scan_function_get()` |
605605
| **Type safety** | Frozen dataclasses with runtime validation |
606606

@@ -649,23 +649,61 @@ constraint_record(
649649
)
650650
```
651651

652-
DuckDB checks three things when it attaches the catalog, and `Table` checks the
653-
same at definition time (the worker checks again when serving each table):
654-
655-
- It reads only `ENFORCED` + `ALL_ROWS` records; state one only when every row
656-
the table can ever return satisfies it.
657-
- Every Arrow field — descending through ordinary structs, but not into
658-
extension or logical-type storage — is non-nullable exactly when a
659-
`NOT_NULL` or `PRIMARY_KEY` record names its path. This applies to tables
660-
with no constraints too: a non-nullable field there is an error.
661-
- The records are readable only under the negotiated
662-
`vgi.constraint.records.read.v1` profile (plus `vgi.expression.duckdb.scalar.v1`
663-
for CHECK). `ReadOnlyCatalogInterface` advertises both automatically when any
664-
declared table carries records; a catalog that serves tables some other way
665-
overrides `catalog_capabilities()`.
652+
DuckDB rejects a table at ATTACH unless its records follow the extension's
653+
rules. `Table` checks them at definition time, `ReadOnlyCatalogInterface` checks
654+
ownership and foreign-key targets across the catalog when it builds its
655+
registries, and the worker checks every table again when serving it (so a
656+
custom catalog's own unit tests and RPC calls see the same errors):
657+
658+
- Only `ENFORCED` + `ALL_ROWS` records; state one only when every row the
659+
table can ever return satisfies it.
660+
- Each record is owned by its table and schema; ids are unique, declared names
661+
are unique case-insensitively, and there is at most one `PRIMARY_KEY`.
662+
- Column paths resolve. Only `NOT_NULL` may name a nested struct child;
663+
`PRIMARY_KEY`, `UNIQUE` and `FOREIGN_KEY` name top-level columns, never the
664+
`is_row_id` column.
665+
- A `FOREIGN_KEY` references a served table in the same schema, whose
666+
`PRIMARY_KEY` or `UNIQUE` record has the referenced id and columns, with
667+
`MATCH SIMPLE` and `NO_ACTION`/`RESTRICT` actions.
668+
- A `CHECK` is a single comparison between signed integer or floating-point
669+
columns and numeric constants, such as `amount >= 0`. The grammar is checked
670+
with DuckDB's own parser when `haybarn` or `duckdb` is installed.
671+
672+
**Nullability is derived, not declared.** DuckDB requires every Arrow field
673+
(descending through ordinary structs, but not into extension or logical-type
674+
storage) to be non-nullable exactly when a `NOT_NULL` or `PRIMARY_KEY` record
675+
names it. The worker serves the schema that way: a non-nullable field no record
676+
names is served nullable, and a field a record names is served non-nullable.
677+
Arrow nullability in `columns` or a function's output schema is therefore never
678+
a constraint claim, and there is nothing to keep in sync by hand.
679+
`served_table_schema()` shows the schema a table is served with.
680+
681+
**Negotiation.** Records are readable only under
682+
`vgi.constraint.records.read.v1` (plus `vgi.expression.duckdb.scalar.v1` for
683+
CHECK), and DuckDB writes a constrained table only under
684+
`vgi.constraint.enforce.v1`. `ReadOnlyCatalogInterface` advertises the read
685+
profiles when any declared table carries records, and the enforcement profile
686+
when such a table also has write functions: records on a writable table attest
687+
that the write functions enforce them. A catalog that serves tables some other
688+
way returns `catalog_capabilities_for(tables, indexes)` from its
689+
`catalog_capabilities()` override (`union` it with anything else it
690+
implements). If enforcement is not negotiated, the worker serves a constrained
691+
table read-only and logs a warning saying why.
666692

667693
The legacy `not_null`, `unique`, `check`, `primary_key` and `foreign_key`
668-
fields are rejected; replace them with records.
694+
fields type-check only as `()` and raise when set; `ForeignKeyDef` and
695+
`ForeignKeyInfo` are deprecated. Replace them with records.
696+
697+
### Generated columns
698+
699+
DuckDB reads only VIRTUAL generated columns of type Boolean, int8–int64 or
700+
string, with no default and no extension type, whose expression uses columns,
701+
Boolean/integer/string literals, `CAST` to those types, comparisons,
702+
`AND`/`OR`/`NOT`, `IS [NOT] NULL`, `COALESCE`, `+ - * %`, `||`, `abs`, `lower`
703+
and `upper`. A generated column must be nullable; one no record names is
704+
served nullable automatically, and naming it in a `NOT_NULL` or `PRIMARY_KEY`
705+
record is an error. Legacy `generated_expression` field metadata is translated
706+
to the canonical record when it is the field's only expression metadata.
669707

670708
### Validation
671709

@@ -687,11 +725,17 @@ Table(
687725
),
688726
)
689727

690-
# Error: Arrow nullability must match NOT_NULL/PRIMARY_KEY records exactly
728+
# Error: more than one PRIMARY_KEY record
691729
Table(
692730
name="users",
693-
columns=pa.schema([pa.field("id", pa.int64(), nullable=False)]),
694-
) # ValueError: ... the field is non-nullable but no ... record names it
731+
columns=pa.schema([("id", pa.int64()), ("email", pa.string())]),
732+
constraints=(
733+
constraint_record(["main"], "users", "pk1", "PRIMARY_KEY", [["id"]],
734+
enforcement="ENFORCED", validation="ALL_ROWS"),
735+
constraint_record(["main"], "users", "pk2", "PRIMARY_KEY", [["email"]],
736+
enforcement="ENFORCED", validation="ALL_ROWS"),
737+
),
738+
) # ValueError: Table 'users': more than one PRIMARY_KEY
695739

696740
# Error: default_schema not in schemas
697741
Catalog(
@@ -1043,6 +1087,12 @@ The current CatalogInterface has the following limitations:
10431087
- **Tags**: Cannot be updated after object creation
10441088
- **Schema metadata**: Comments and tags cannot be updated on schemas
10451089
- **Constraints**: Only NOT NULL can be added/dropped (no ALTER for UNIQUE/CHECK)
1090+
- **Constrained CREATE TABLE**: `CREATE TABLE` with `NOT NULL`, `PRIMARY KEY`,
1091+
`UNIQUE`, `CHECK` or `FOREIGN KEY` reaches a catalog only when it advertises
1092+
`vgi.constraint.create.v1` and `vgi.constraint.enforce.v1`, i.e. when its
1093+
`table_create()` stores the records and its write paths enforce them. The SDK
1094+
ships no catalog that does (the in-memory test catalog stores no rows), so
1095+
such statements are refused with an error naming the missing profile.
10461096

10471097
### Table writes and indexes
10481098

@@ -1067,9 +1117,33 @@ on declarative tables. Table writes do not require the bundled
10671117
[transactor](api/transactor.md); that is an optional database-access service with
10681118
additional engine requirements.
10691119

1070-
Index metadata can be declared with [`Index`][vgi.catalog.descriptors.Index]. Custom
1071-
catalogs can implement `index_get()`, `index_create()`, and `index_drop()`; support
1072-
depends on the catalog implementation and backing data source.
1120+
Index metadata can be declared with [`Index`][vgi.catalog.descriptors.Index],
1121+
building its definition with `index_definition()`:
1122+
1123+
```python test="skip"
1124+
from vgi.catalog import Index, index_definition
1125+
1126+
Index(name="orders_customer", definition=index_definition(["main"], "orders", [["customer_id"]], algorithm="art"))
1127+
```
1128+
1129+
`ReadOnlyCatalogInterface` advertises `vgi.index.definition.read.v1` when any
1130+
schema declares an index. Custom catalogs can implement `index_get()`,
1131+
`index_create()`, and `index_drop()`; support depends on the catalog
1132+
implementation and backing data source.
1133+
1134+
### Overrides written for older signatures
1135+
1136+
vgi.v3 added keyword arguments to several `CatalogInterface` methods
1137+
(`catalog_attach(client_capabilities=)`, `schema_create(options=)`,
1138+
`view_create(aliases=, security=, comment=, tags=, column_comments=)`,
1139+
`table_create(constraint_definitions=, ...)`) and renamed `column_name` to
1140+
`column_path`. The worker inspects an override's signature and passes only
1141+
what it accepts, emitting a `DeprecationWarning` (and one log line) when it
1142+
drops an argument. It drops one only when the value carries nothing the old
1143+
override could have honoured (`None`, empty, a top-level column path); a
1144+
barrier view, schema options or a nested column path fail with an error naming
1145+
the parameter instead of being silently ignored. Update overrides to the
1146+
current signatures, or accept `**kwargs`.
10731147

10741148
---
10751149

‎tests/catalog/test_constraint_example_cross_sdk.py‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,19 @@
1111
import pytest
1212

1313
from vgi.catalog import SchemaObjectType
14-
from vgi.catalog.constraints import ConstraintEnforcement, ConstraintValidation, validate_constraint_schema
14+
from vgi.catalog.constraints import (
15+
ConstraintEnforcement,
16+
ConstraintValidation,
17+
served_table_schema,
18+
validate_constraint_records,
19+
)
1520
from vgi.client import Client
1621

1722
WORKERS: dict[str, list[str]] = json.loads(os.environ.get("VGI_METADATA_WORKERS", "{}"))
1823

1924

2025
@pytest.mark.parametrize("sdk", sorted(WORKERS))
21-
def test_example_constraints_are_explicit_stable_untrusted_records(sdk: str) -> None:
26+
def test_example_constraints_are_explicit_stable_trusted_records(sdk: str) -> None:
2227
with Client(server_path=WORKERS[sdk], worker_limit=1) as client:
2328
token = client.catalog_attach(
2429
name="example", data_version_spec=None, implementation_version=None
@@ -32,11 +37,11 @@ def test_example_constraints_are_explicit_stable_untrusted_records(sdk: str) ->
3237
assert fetched.constraints == listed.constraints
3338
infos = fetched.constraint_infos()
3439
assert infos, (sdk, name)
35-
assert all(item.enforcement == ConstraintEnforcement.METADATA_ONLY for item in infos)
36-
assert all(item.validation == ConstraintValidation.UNVALIDATED for item in infos)
37-
assert not any(item.trusted for item in infos)
38-
validate_constraint_schema(
39-
pa.ipc.read_schema(pa.py_buffer(fetched.columns)),
40-
[item.definition for item in infos],
41-
trusted=[],
42-
)
40+
# DuckDB reads only ENFORCED + ALL_ROWS records; anything else fails ATTACH.
41+
assert all(item.enforcement == ConstraintEnforcement.ENFORCED for item in infos)
42+
assert all(item.validation == ConstraintValidation.ALL_ROWS for item in infos)
43+
assert all(item.trusted for item in infos)
44+
columns = pa.ipc.read_schema(pa.py_buffer(fetched.columns))
45+
validate_constraint_records(name, columns, infos, schema_path=["data"])
46+
# Served nullability already equals NOT_NULL/PRIMARY_KEY membership.
47+
assert served_table_schema(columns, infos).equals(columns, check_metadata=True)

‎tests/catalog/test_constraint_public_contract.py‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -170,12 +170,12 @@ def test_discovery_never_hides_constraints_and_mutation_requires_revision() -> N
170170
Worker._table_for_capabilities(table_info(definition_revision=None), PROFILES)
171171

172172

173-
def test_discovery_rejects_nullability_that_does_not_match_records() -> None:
174-
wrong = pa.schema([pa.field("a.b", pa.int64(), nullable=True)])
175-
with pytest.raises(ValueError, match="nullability"):
176-
Worker._table_for_capabilities(
177-
table_info(columns=wrong.serialize().to_pybytes(), constraints=[INFO.serialize_to_bytes()]), PROFILES
178-
)
173+
def test_discovery_serves_record_nullability() -> None:
174+
nullable = pa.schema([pa.field("a.b", pa.int64(), nullable=True)])
175+
served = Worker._table_for_capabilities(
176+
table_info(columns=nullable.serialize().to_pybytes(), constraints=[INFO.serialize_to_bytes()]), PROFILES
177+
)
178+
assert not pa.ipc.read_schema(pa.py_buffer(served.columns)).field("a.b").nullable
179179

180180

181181
@pytest.mark.parametrize(

‎tests/catalog/test_constraint_records.py‎

Lines changed: 44 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@
77
profile was negotiated, (b) every record is ENFORCED + ALL_ROWS and (c) Arrow
88
nullability equals membership in the NOT_NULL/PRIMARY_KEY paths. These tests
99
pin that a declarative catalog satisfies (a) with no hand-written capability
10-
override and that (b) and (c) fail in the worker's own process.
10+
override, that (b) fails in the worker's own process, and that the worker
11+
derives (c) from the records instead of failing.
1112
"""
1213

1314
from __future__ import annotations
@@ -186,39 +187,59 @@ def test_record_for_another_table_fails_at_definition() -> None:
186187
Table(name="places", columns=COLUMNS, constraints=(other, *RECORDS[1:]))
187188

188189

189-
# --- (c) Arrow nullability equals NOT_NULL/PRIMARY_KEY membership ---
190+
# --- (c) served Arrow nullability is derived from NOT_NULL/PRIMARY_KEY records ---
190191

191192

192-
def test_non_nullable_field_without_record_fails_even_without_constraints() -> None:
193-
with pytest.raises(ValueError, match=r"Table 't': .*column \['id'\]: the field is non-nullable but no"):
194-
Table(name="t", columns=pa.schema([pa.field("id", pa.int64(), nullable=False)]))
193+
def _served(columns: pa.Schema, records: tuple[ConstraintInfo, ...] = (), name: str = "places") -> pa.Schema:
194+
info = TableInfo(
195+
name=name,
196+
schema_path=["main"],
197+
comment=None,
198+
tags={},
199+
columns=SerializedSchema(columns.serialize().to_pybytes()),
200+
constraints=[record.serialize_to_bytes() for record in records],
201+
)
202+
served = Worker._table_for_capabilities(info, CatalogCapabilities(read=sorted(PROFILES)))
203+
return pa.ipc.read_schema(pa.py_buffer(served.columns))
204+
195205

206+
def test_unnamed_non_nullable_field_is_served_nullable() -> None:
207+
Table(name="t", columns=pa.schema([pa.field("id", pa.int64(), nullable=False)]))
208+
served = _served(pa.schema([pa.field("id", pa.int64(), nullable=False)]), name="t")
209+
assert served.field("id").nullable
196210

197-
def test_nullable_field_with_record_fails() -> None:
211+
212+
def test_record_named_nullable_field_is_served_non_nullable() -> None:
198213
record = constraint_record(["main"], "t", "nn", "NOT_NULL", [["id"]], **TRUSTED)
199-
with pytest.raises(ValueError, match=r"Table 't': .*column \['id'\]: the field is nullable but"):
200-
Table(name="t", columns=pa.schema([("id", pa.int64())]), constraints=(record,))
214+
Table(name="t", columns=pa.schema([("id", pa.int64())]), constraints=(record,))
215+
assert not _served(pa.schema([("id", pa.int64())]), (record,), name="t").field("id").nullable
201216

202217

203-
def test_nested_struct_child_nullability_is_checked_both_ways() -> None:
204-
with pytest.raises(ValueError, match=r"column \['bbox', 'xmin'\]: the field is non-nullable but no"):
205-
Table(name="places", columns=COLUMNS, constraints=(RECORDS[0], RECORDS[2]))
218+
def test_nested_struct_child_nullability_follows_records() -> None:
219+
# No record names bbox.xmin: the non-nullable child is served nullable.
220+
served = _served(COLUMNS, (RECORDS[0], RECORDS[2]))
221+
assert served.field("bbox").type.field("xmin").nullable
222+
assert not served.field("id").nullable
223+
# A record names a nullable child: it is served non-nullable; the parent stays nullable.
206224
nullable_child = pa.schema([COLUMNS.field("id"), pa.field("bbox", pa.struct([("xmin", pa.float64())]))])
207-
with pytest.raises(ValueError, match=r"column \['bbox', 'xmin'\]: the field is nullable but"):
208-
Table(name="places", columns=nullable_child, constraints=RECORDS[:2])
225+
served = _served(nullable_child, RECORDS[:2])
226+
assert not served.field("bbox").type.field("xmin").nullable
227+
assert served.field("bbox").nullable
209228

210229

211-
def test_nested_mismatch_fails_when_served() -> None:
212-
info = TableInfo(
213-
name="places",
214-
schema_path=["main"],
215-
comment=None,
216-
tags={},
217-
columns=SerializedSchema(COLUMNS.serialize().to_pybytes()),
218-
constraints=[RECORDS[0].serialize_to_bytes()],
230+
def test_row_id_and_logical_storage_are_untouched() -> None:
231+
storage = pa.struct([pa.field("x", pa.float64(), nullable=False)])
232+
columns = pa.schema(
233+
[
234+
pa.field("rowid", pa.int64(), nullable=False, metadata={b"is_row_id": b"1"}),
235+
pa.field("point", storage, nullable=False, metadata={b"ARROW:extension:name": b"example.point"}),
236+
]
219237
)
220-
with pytest.raises(ValueError, match=r"Table 'places': Arrow nullability disagrees .* \['bbox', 'xmin'\]"):
221-
Worker._table_for_capabilities(info, CatalogCapabilities(read=sorted(PROFILES)))
238+
served = _served(columns, name="t")
239+
assert not served.field("rowid").nullable
240+
assert served.field("point").nullable
241+
assert not served.field("point").type.field("x").nullable
242+
assert served.field("rowid").metadata == {b"is_row_id": b"1"}
222243

223244

224245
@pytest.mark.parametrize(

‎tests/catalog/test_generated_columns_protocol.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,13 @@ def test_unsupported_generation_rejects_before_provider_and_during_discovery(
198198
tags={},
199199
)
200200
monkeypatch.setattr(backend._get_catalog(), "table_get", lambda **kwargs: info)
201+
if b"generated_expression" in (field.metadata or {}):
202+
# Discovery translates unambiguous top-level legacy metadata to the
203+
# canonical record instead of rejecting the table; CREATE still refuses it.
204+
served = backend.catalog_table_get(**args(token)).to_optional()
205+
expressions = column_expressions(pa.ipc.read_schema(pa.py_buffer(served.columns)).field("g"))
206+
assert expressions.generated is not None and expressions.generated.sql == "n + 1"
207+
return
201208
with pytest.raises(error, match=message):
202209
backend.catalog_table_get(**args(token))
203210

0 commit comments

Comments
 (0)