Skip to content

Commit b9c621a

Browse files
rustyconoverclaude
andcommitted
feat(catalog): unify required_field_filter_paths -> required_filters (AND-of-ORs)
Replace the flat, AND-only required_field_filter_paths with a single required_filters field in conjunctive normal form: an AND (outer list) of OR-groups (inner lists) of dotted-path column references. A group is satisfied when any one of its paths has a filter; every group must be satisfied. So (("accession_number",), ("ticker","cik")) means "accession_number AND one of (ticker, cik)" — the ticker-OR-cik requirement the old flat list couldn't express. Wire type becomes list<list<utf8>> (auto-derived from the annotation; mirrors unique_constraints), kept as the trailing TableInfo field so older extensions degrade gracefully to no-enforcement. Updates the Table descriptor + validation, TableInfo, the rff_* fixtures (singleton groups preserve prior AND behavior) plus a new rff_or OR-group fixture, and the catalog tests. No backward compatibility (clean rename). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 43974b5 commit b9c621a

8 files changed

Lines changed: 206 additions & 185 deletions

File tree

‎tests/catalog/test_required_field_filter_paths.py‎

Lines changed: 0 additions & 98 deletions
This file was deleted.
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
"""Unit tests for the ``Table.required_filters`` field.
2+
3+
The field is purely declarative on the Python side — enforcement runs in the C++
4+
optimizer extension. It is conjunctive normal form: an AND (outer tuple) of
5+
OR-groups (inner tuples) of dotted-path column references. These tests cover:
6+
7+
1. Default-empty behaviour (no field set → ``TableInfo`` ships an empty list).
8+
2. Populated round-trip (singleton groups + a multi-path OR-group preserved).
9+
3. Validation: each path's leading dotted segment must be a real column.
10+
4. Empty-string paths and empty OR-groups are rejected loudly.
11+
"""
12+
13+
from __future__ import annotations
14+
15+
import pyarrow as pa
16+
import pytest
17+
18+
from vgi.catalog.descriptors import Table
19+
20+
21+
class TestRequiredFiltersField:
22+
"""Behaviour of the declarative required_filters field."""
23+
24+
def _bbox_columns(self) -> pa.Schema:
25+
"""Return a small schema with a ``bbox`` STRUCT plus flat id/ticker."""
26+
return pa.schema(
27+
[
28+
("id", pa.int64()),
29+
("ticker", pa.string()),
30+
(
31+
"bbox",
32+
pa.struct(
33+
[
34+
("xmin", pa.float64()),
35+
("ymin", pa.float64()),
36+
("xmax", pa.float64()),
37+
("ymax", pa.float64()),
38+
]
39+
),
40+
),
41+
]
42+
)
43+
44+
def test_default_is_empty_tuple(self) -> None:
45+
"""Default is an empty tuple; TableInfo wire field is an empty list."""
46+
t = Table(name="t", columns=self._bbox_columns())
47+
assert t.required_filters == ()
48+
info = t.to_table_info("main")
49+
assert info.required_filters == []
50+
51+
def test_singleton_groups_round_trip(self) -> None:
52+
"""Singleton groups (the AND-only case) survive as a list of lists."""
53+
groups = (("bbox.xmin",), ("bbox.xmax",), ("bbox.ymin",), ("bbox.ymax",))
54+
t = Table(name="place", columns=self._bbox_columns(), required_filters=groups)
55+
assert t.required_filters == groups
56+
info = t.to_table_info("main")
57+
assert info.required_filters == [list(g) for g in groups]
58+
59+
def test_or_group_round_trip(self) -> None:
60+
"""A multi-path OR-group survives alongside a singleton mandatory group."""
61+
groups = (("id",), ("id", "ticker"))
62+
t = Table(name="t", columns=self._bbox_columns(), required_filters=groups)
63+
info = t.to_table_info("main")
64+
assert info.required_filters == [["id"], ["id", "ticker"]]
65+
66+
def test_top_level_path(self) -> None:
67+
"""A top-level column name (no dots) is a valid path."""
68+
t = Table(name="t", columns=self._bbox_columns(), required_filters=(("id",),))
69+
assert t.to_table_info("main").required_filters == [["id"]]
70+
71+
def test_nested_path_is_not_unpacked_for_validation(self) -> None:
72+
"""Only the leading dotted segment is validated; deeper segments pass through."""
73+
# Subfield validity isn't checked here (descriptor doesn't unpack struct
74+
# schemas) — only the leading segment matters. Typos like ``bbox.nope``
75+
# go through and DuckDB catches them at scan time.
76+
t = Table(name="t", columns=self._bbox_columns(), required_filters=(("bbox.nope",),))
77+
assert t.required_filters == (("bbox.nope",),)
78+
79+
def test_unknown_leading_segment_raises(self) -> None:
80+
"""A path whose leading segment is not a real column is rejected."""
81+
with pytest.raises(ValueError, match=r"unknown column 'nope'"):
82+
Table(name="t", columns=self._bbox_columns(), required_filters=(("nope.xmin",),))
83+
84+
def test_empty_string_path_raises(self) -> None:
85+
"""An empty string path is rejected loudly rather than silently accepted."""
86+
with pytest.raises(ValueError, match="must not contain empty strings"):
87+
Table(name="t", columns=self._bbox_columns(), required_filters=(("",),))
88+
89+
def test_empty_group_raises(self) -> None:
90+
"""An empty OR-group is rejected loudly."""
91+
with pytest.raises(ValueError, match="must not contain empty groups"):
92+
Table(name="t", columns=self._bbox_columns(), required_filters=((),))

‎tests/catalog/test_serialization.py‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1149,10 +1149,10 @@ def test_table_info_schema(self) -> None:
11491149
assert schema.field("cardinality_estimate").type == pa.int64()
11501150
assert schema.field("column_statistics").type == pa.binary()
11511151
assert schema.field("bind_result").type == pa.binary()
1152-
assert schema.field("required_field_filter_paths").type == pa.list_(pa.string())
1153-
# required_field_filter_paths is the LAST field (positional schema
1154-
# compatibility: old C++ extensions ignore-by-position trailing fields).
1155-
assert schema.field(len(schema) - 1).name == "required_field_filter_paths"
1152+
assert schema.field("required_filters").type == pa.list_(pa.list_(pa.string()))
1153+
# required_filters is the LAST field (positional schema compatibility:
1154+
# old C++ extensions ignore-by-position trailing fields).
1155+
assert schema.field(len(schema) - 1).name == "required_filters"
11561156

11571157
def test_view_info_schema(self) -> None:
11581158
"""Verify ViewInfo Arrow schema."""

‎tests/landing/describe.expected.json‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
"counts": {
66
"functions": 183,
77
"schemas": 2,
8-
"tables": 58,
8+
"tables": 59,
99
"views": 3
1010
},
1111
"data_version_spec": null,
@@ -225,9 +225,14 @@
225225
},
226226
{
227227
"cols": 2,
228-
"comment": "rff_none — control table with no required_field_filter_paths (opt-out fast path).",
228+
"comment": "rff_none — control table with no required_filters (opt-out fast path).",
229229
"name": "rff_none"
230230
},
231+
{
232+
"cols": 2,
233+
"comment": "rff_or — requires a filter on 'a' OR 'b' (a single OR-group).",
234+
"name": "rff_or"
235+
},
231236
{
232237
"cols": 2,
233238
"comment": "rff_parquet — native read_parquet delegation with bbox.* required filters.",
@@ -2316,13 +2321,13 @@
23162321
},
23172322
{
23182323
"args": [],
2319-
"doc": "rff_nested — nested STRUCT(wrapper.mid.leaf) for required_field_filter_paths tests",
2324+
"doc": "rff_nested — nested STRUCT(wrapper.mid.leaf) for required_filters tests",
23202325
"name": "rff_nested_scan",
23212326
"type": "table"
23222327
},
23232328
{
23242329
"args": [],
2325-
"doc": "rff_none — control table with no required_field_filter_paths",
2330+
"doc": "rff_none — control table with no required_filters",
23262331
"name": "rff_none_scan",
23272332
"type": "table"
23282333
},
@@ -2334,13 +2339,13 @@
23342339
},
23352340
{
23362341
"args": [],
2337-
"doc": "rff_simple — flat columns (a, b) for required_field_filter_paths tests",
2342+
"doc": "rff_simple — flat columns (a, b) for required_filters tests",
23382343
"name": "rff_simple_scan",
23392344
"type": "table"
23402345
},
23412346
{
23422347
"args": [],
2343-
"doc": "rff_struct — STRUCT(s.a, s.b) + other for required_field_filter_paths tests",
2348+
"doc": "rff_struct — STRUCT(s.a, s.b) + other for required_filters tests",
23442349
"name": "rff_struct_scan",
23452350
"type": "table"
23462351
},

‎vgi/_test_fixtures/table/required_filters.py‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"""Scan functions backing the ``rff_*`` required-filter sqllogictest Tables.
44
55
Used by the ``vgi_required_filters_*.test`` matrix. These fixtures exercise
6-
the ``Table.required_field_filter_paths`` field +
6+
the ``Table.required_filters`` field +
77
the C++ optimizer extension that enforces it. The five tables form a small
88
matrix:
99
@@ -99,10 +99,10 @@
9999
)
100100

101101
# rff_rowid — a row-id column (virtual, hidden from SELECT *) alongside a bbox
102-
# struct with required_field_filter_paths. A `WHERE rowid = N` predicate pushes
102+
# struct with required_filters. A `WHERE rowid = N` predicate pushes
103103
# a table_filter keyed by the COLUMN_IDENTIFIER_ROW_ID sentinel (>> column
104104
# count), which the optimizer's required-filter check must skip rather than
105-
# index out of bounds. See required_field_filter_paths_native.test.
105+
# index out of bounds. See required_filters_native.test.
106106
RFF_ROWID_COLUMNS = pa.schema(
107107
[
108108
pa.field("row_id", pa.int64(), metadata={b"is_row_id": b""}),
@@ -124,7 +124,7 @@
124124

125125
RffSimpleScanFunction = _static_scan_function(
126126
func_name="rff_simple_scan",
127-
func_description="rff_simple — flat columns (a, b) for required_field_filter_paths tests",
127+
func_description="rff_simple — flat columns (a, b) for required_filters tests",
128128
output_schema=RFF_SIMPLE_COLUMNS,
129129
data={
130130
"a": [1, 2, 3],
@@ -134,7 +134,7 @@
134134

135135
RffStructScanFunction = _static_scan_function(
136136
func_name="rff_struct_scan",
137-
func_description="rff_struct — STRUCT(s.a, s.b) + other for required_field_filter_paths tests",
137+
func_description="rff_struct — STRUCT(s.a, s.b) + other for required_filters tests",
138138
output_schema=RFF_STRUCT_COLUMNS,
139139
data={
140140
"s": [
@@ -148,7 +148,7 @@
148148

149149
RffNestedScanFunction = _static_scan_function(
150150
func_name="rff_nested_scan",
151-
func_description="rff_nested — nested STRUCT(wrapper.mid.leaf) for required_field_filter_paths tests",
151+
func_description="rff_nested — nested STRUCT(wrapper.mid.leaf) for required_filters tests",
152152
output_schema=RFF_NESTED_COLUMNS,
153153
data={
154154
"wrapper": [
@@ -174,7 +174,7 @@
174174

175175
RffNoneScanFunction = _static_scan_function(
176176
func_name="rff_none_scan",
177-
func_description="rff_none — control table with no required_field_filter_paths",
177+
func_description="rff_none — control table with no required_filters",
178178
output_schema=RFF_NONE_COLUMNS,
179179
data={
180180
"a": [1, 2, 3],

0 commit comments

Comments
 (0)