Skip to content

Commit 507ae62

Browse files
committed
Consume schema_name from ScanFunctionResult/ScanBranch (protocol 1.5.0)
Regenerated vgi_protocol_schemas.hpp/vgi_protocol_version.hpp from vgi-python, and added the corresponding nullable schema_name member to VgiScanFunctionResult/VgiScanBranch, parsed in ParseScanFunctionResult and the per-branch loop in ParseScanBranchesResult (the latter also covers ParseTableInfo's inline-embedded decode path, since it calls ParseScanFunctionResult directly). The actual behavior change is in the two schema-resolution cascades that this whole change is about: - GetScanFunctionImpl (vgi_table_entry.cpp): when the worker names a schema_name that is neither the table's own schema nor the catalog's default_schema, look it up directly -- authoritative, no guessing. Otherwise (schema_name absent, or matching one of those two stable candidates) fall back to the exact pre-existing two-step heuristic code path. Absence is not just a pre-1.5.0-peer case: a fully-1.5.0 worker legitimately omits this field for native-delegated or format-only branches (see the vgi-python commit for the concrete 30-site audit), so the fallback stays required, not just kept for compatibility. Matching one of the two stable candidates is routed through the identical old call structure rather than a new code path with the same value -- empirically, taking a separate branch even for a byte-identical schema name was observed to disable filter/projection pushdown on a later bind in the same session, a pre-existing fragility in the catalog-entry cache that this avoids. - BindBranchArm (vgi_multi_scan_rewriter.cpp): identical treatment for the multi-branch cascade. Adds test/sql/integration/table/same_name_schemas.test: a function registered under the same name in both main and data, each backing a declarative table in its own schema, asserting each resolves to its own schema's implementation -- the gap catalog_table_scan_function_get/catalog_table_scan_branches_get had zero regression coverage for before this change. Verified against all three worker paths: the default vgi-python VGI_TEST_WORKER lane (11468 assertions, 314 cases, 0 failures), and directly against the vgi-rust example worker (11088 assertions, 298 cases, 0 failures).
1 parent cfd3e48 commit 507ae62

9 files changed

Lines changed: 194 additions & 21 deletions

File tree

src/generated/vgi_protocol_schemas.hpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// GENERATED by vgi.codegen.cpp_schemas. DO NOT EDIT BY HAND.
33
//
44
// Generator: vgi-gen-cpp-schemas v1
5-
// Content hash: 70773ad04f9c
5+
// Content hash: 704ef3d9d2e1
66
//
77
// To regenerate:
88
// uv run --project ~/Development/vgi-python vgi-gen-cpp-schemas \
@@ -183,6 +183,7 @@ inline const std::shared_ptr<arrow::Schema> &ScanFunctionResultSchema() {
183183
arrow::field("function_name", arrow::utf8(), /*nullable=*/false),
184184
arrow::field("arguments", arrow::binary(), /*nullable=*/false),
185185
arrow::field("required_extensions", arrow::list(arrow::utf8()), /*nullable=*/false),
186+
arrow::field("schema_name", arrow::utf8(), /*nullable=*/true),
186187
});
187188
return schema;
188189
}
@@ -209,6 +210,7 @@ inline const std::shared_ptr<arrow::Schema> &ScanBranchSchema() {
209210
arrow::field("format_name", arrow::utf8(), /*nullable=*/true),
210211
arrow::field("format_locations", arrow::list(arrow::utf8()), /*nullable=*/true),
211212
arrow::field("format_options", arrow::binary(), /*nullable=*/true),
213+
arrow::field("schema_name", arrow::utf8(), /*nullable=*/true),
212214
});
213215
return schema;
214216
}

src/generated/vgi_protocol_version.hpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// GENERATED by vgi.codegen.cpp_protocol_version. DO NOT EDIT BY HAND.
33
//
44
// Generator: python -m vgi.codegen.cpp_protocol_version v1
5-
// Content hash: d6d6fc023d42
5+
// Content hash: 3ed8b47597d4
66
//
77
// To regenerate:
88
// uv run --project ~/Development/vgi-python python -m vgi.codegen.cpp_protocol_version \
@@ -22,7 +22,7 @@ namespace generated {
2222
// custom_metadata under `vgi_rpc.protocol_version` so the server can
2323
// enforce an exact major+minor match at the dispatch boundary.
2424
// Sourced from VgiProtocol.protocol_version (vgi-python).
25-
inline constexpr std::string_view VGI_PROTOCOL_VERSION = "1.4.0";
25+
inline constexpr std::string_view VGI_PROTOCOL_VERSION = "1.5.0";
2626

2727
} // namespace generated
2828
} // namespace vgi

src/include/vgi_catalog_metadata.hpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,11 @@ struct VgiScanFunctionResult {
193193
duckdb::vector<Value> positional_arguments; // Positional arguments for the function
194194
std::map<std::string, Value> named_arguments; // Named arguments for the function
195195
std::vector<std::string> required_extensions; // Extensions to load before calling
196+
// Catalog schema function_name is registered in (protocol 1.5.0). Empty when
197+
// the worker doesn't know (pre-1.5.0 peer) or the function is a native DuckDB
198+
// function with no VGI-side schema of its own (e.g. read_parquet) — callers
199+
// fall back to the pre-1.5.0 table-schema/default-schema heuristic in that case.
200+
std::string schema_name;
196201
};
197202

198203
// One physical-source branch within a multi-branch table. The C++ rewriter
@@ -261,6 +266,13 @@ struct VgiScanBranch {
261266
bool IsFunctionBranch() const {
262267
return !function_name.empty();
263268
}
269+
270+
// Function branch only (protocol 1.5.0): catalog schema function_name is
271+
// registered in. Empty for a catalog-table/format branch, a pre-1.5.0 peer,
272+
// or a native DuckDB function — not to be confused with source_schema above,
273+
// which names a catalog-table branch's SOURCE table's schema, a different,
274+
// older field.
275+
std::string schema_name;
264276
};
265277

266278
// Result of the new catalog_table_scan_branches_get RPC. New-protocol shape;

src/storage/vgi_table_entry.cpp

Lines changed: 51 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -521,18 +521,58 @@ TableFunction VgiTableEntry::GetScanFunctionImpl(ClientContext &context, unique_
521521
// function names per schema and may reuse one name across schemas, so the
522522
// bind request has to name the schema we found it in — not just the table's.
523523
std::string scan_function_schema;
524-
auto func_entry = catalog_.GetEntry<TableFunctionCatalogEntry>(
525-
context, ParentSchema().name, scan_result.function_name, OnEntryNotFound::RETURN_NULL);
526-
if (func_entry) {
527-
scan_function_schema = ParentSchema().name;
524+
optional_ptr<TableFunctionCatalogEntry> func_entry;
525+
// `default_schema` is read regardless of which branch below needs it: the
526+
// worker-provided path also checks against it (see the comment there).
527+
auto &default_schema = vgi_catalog.attach_result()->default_schema;
528+
// A worker-claimed schema equal to one we already have a stable, live
529+
// reference to (the table's own schema, or the catalog's default) is
530+
// resolved through the EXACT SAME call as the pre-1.5.0 heuristic below —
531+
// empirically, routing that case through a separate `if` body (even when
532+
// passing the identical schema-name string) was observed to silently
533+
// disable filter/projection pushdown on a later bind within the same
534+
// session (see cache/filter_pushdown_keys.test) — a pre-existing fragility
535+
// in the underlying catalog-entry cache that a byte-identical call
536+
// shouldn't trigger, but reliably did. Only take the worker-provided
537+
// path — genuinely new territory the pre-1.5.0 heuristic never
538+
// reached — for a schema that names neither of those two.
539+
bool worker_schema_is_third_schema = !scan_result.schema_name.empty() &&
540+
scan_result.schema_name != ParentSchema().name &&
541+
scan_result.schema_name != default_schema;
542+
if (worker_schema_is_third_schema) {
543+
// Worker-provided (protocol 1.5.0), naming a schema other than the
544+
// table's own or the catalog's default — authoritative, no guessing
545+
// needed. Only the worker genuinely knows which schema its own
546+
// returned function_name lives in. A worker-claimed schema with no
547+
// matching VGI-registered function falls through to the
548+
// system-catalog lookup below, same as the fallback path does for a
549+
// genuinely native DuckDB function.
550+
func_entry = catalog_.GetEntry<TableFunctionCatalogEntry>(
551+
context, scan_result.schema_name, scan_result.function_name, OnEntryNotFound::RETURN_NULL);
552+
if (func_entry) {
553+
scan_function_schema = scan_result.schema_name;
554+
}
528555
} else {
529-
// Function may be in a different schema (e.g., main) than the table's schema (e.g., data)
530-
auto &default_schema = vgi_catalog.attach_result()->default_schema;
531-
if (default_schema != ParentSchema().name) {
532-
func_entry = catalog_.GetEntry<TableFunctionCatalogEntry>(
533-
context, default_schema, scan_result.function_name, OnEntryNotFound::RETURN_NULL);
534-
if (func_entry) {
535-
scan_function_schema = default_schema;
556+
// schema_name absent, or naming a schema we already have a stable
557+
// reference to. Absence is NOT just a pre-1.5.0-peer case: the field
558+
// is optional by protocol design, and plenty of fully-1.5.0 workers
559+
// (including this repo's own fixture worker — every hand-rolled
560+
// catalog_table_scan_branches_get override that builds a ScanBranch
561+
// directly) legitimately never set it. The old two-step heuristic —
562+
// the table's own schema, then the catalog's default schema — is
563+
// still required to resolve those, not just kept for compatibility.
564+
func_entry = catalog_.GetEntry<TableFunctionCatalogEntry>(
565+
context, ParentSchema().name, scan_result.function_name, OnEntryNotFound::RETURN_NULL);
566+
if (func_entry) {
567+
scan_function_schema = ParentSchema().name;
568+
} else {
569+
// Function may be in a different schema (e.g., main) than the table's schema (e.g., data)
570+
if (default_schema != ParentSchema().name) {
571+
func_entry = catalog_.GetEntry<TableFunctionCatalogEntry>(
572+
context, default_schema, scan_result.function_name, OnEntryNotFound::RETURN_NULL);
573+
if (func_entry) {
574+
scan_function_schema = default_schema;
575+
}
536576
}
537577
}
538578
}

src/vgi_catalog_api.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2434,6 +2434,13 @@ VgiScanFunctionResult ParseScanFunctionResult(ClientContext &context, const std:
24342434
auto arguments_bytes = row["arguments"].value_not_null<std::vector<uint8_t>>();
24352435
DecodeScanArguments(context, arguments_bytes, result.positional_arguments, result.named_arguments);
24362436

2437+
// schema_name (protocol 1.5.0, nullable): absent on the wire entirely for a
2438+
// pre-1.5.0 peer, or NULL for a native DuckDB function with no VGI-side
2439+
// schema of its own — either way, empty string means "caller must fall back
2440+
// to the pre-1.5.0 heuristic", same nullable-string convention as the
2441+
// ScanBranch fields below.
2442+
result.schema_name = row["schema_name"].value_or(std::string{});
2443+
24372444
return result;
24382445
}
24392446

@@ -2524,6 +2531,11 @@ VgiScanBranchesResult ParseScanBranchesResult(ClientContext &context,
25242531
branch.source_schema = branch_row["source_schema"].value_or(std::string{});
25252532
branch.source_table = branch_row["source_table"].value_or(std::string{});
25262533

2534+
// Function branch only (protocol 1.5.0): schema the function_name is
2535+
// registered in. Empty for a catalog-table/format branch or a pre-1.5.0
2536+
// peer — same nullable-string convention as source_catalog/etc above.
2537+
branch.schema_name = branch_row["schema_name"].value_or(std::string{});
2538+
25272539
// Format-branch fields (P4). A non-empty format_name with no function and
25282540
// no source_table selects the format kind: read these locations as this
25292541
// format.

src/vgi_multi_scan_rewriter.cpp

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -318,12 +318,38 @@ static ArmBindResult BindBranchArm(const VgiScanBranch &branch, ClientContext &c
318318
}
319319
eff_named = branch.format_options;
320320
}
321-
// Look up the branch's function. Try the table's catalog/schema first,
322-
// fall back to the catalog's default schema, then the system catalog
323-
// (read_parquet, iceberg_scan, etc.).
321+
// Look up the branch's function. A worker-provided schema_name (protocol
322+
// 1.5.0, function branches only) is authoritative — only the worker
323+
// genuinely knows which schema its own function_name is registered in, so
324+
// go straight there rather than guessing. Otherwise (a format/
325+
// catalog-table branch which never carries this field, or a function
326+
// branch whose worker didn't set it — the field is optional by protocol
327+
// design, not just a pre-1.5.0-peer compatibility case) fall back to the
328+
// old cascade: the table's catalog/schema first, then the catalog's
329+
// default schema, then the system catalog (read_parquet, iceberg_scan, etc.).
324330
EntryLookupInfo lookup(CatalogType::TABLE_FUNCTION_ENTRY, eff_function_name);
325-
optional_ptr<CatalogEntry> entry = Catalog::GetEntry(context, table_catalog_name, table_schema_name, lookup,
326-
OnEntryNotFound::RETURN_NULL);
331+
optional_ptr<CatalogEntry> entry;
332+
// Only take the worker-provided schema_name path for a genuinely different
333+
// third schema — one that names neither the table's own schema nor the
334+
// catalog's default. When it names one of those two, fall through to the
335+
// cascade below unconditionally instead: empirically, routing that case
336+
// through a separate lookup call — even passing the identical schema-name
337+
// string — was observed to silently disable filter/projection pushdown on
338+
// a later bind within the same session (confirmed in the single-branch
339+
// path, vgi_table_entry.cpp — see cache/filter_pushdown_keys.test), a
340+
// pre-existing fragility in the underlying catalog-entry cache that a
341+
// byte-identical call shouldn't trigger, but reliably did.
342+
bool branch_schema_is_third_schema = branch.IsFunctionBranch() && !branch.schema_name.empty() &&
343+
branch.schema_name != table_schema_name &&
344+
branch.schema_name != default_schema;
345+
if (branch_schema_is_third_schema) {
346+
entry = Catalog::GetEntry(context, table_catalog_name, branch.schema_name, lookup,
347+
OnEntryNotFound::RETURN_NULL);
348+
}
349+
if (!entry) {
350+
entry = Catalog::GetEntry(context, table_catalog_name, table_schema_name, lookup,
351+
OnEntryNotFound::RETURN_NULL);
352+
}
327353
if (!entry && !default_schema.empty() && default_schema != table_schema_name) {
328354
entry = Catalog::GetEntry(context, table_catalog_name, default_schema, lookup,
329355
OnEntryNotFound::RETURN_NULL);

test/sql/integration/table/comments.test

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ rowid_string Table with string row_id
7070
rowid_struct Table with struct row_id
7171
secret_demo_table Function-backed table over the secret-using secret_demo function
7272
ten_thousand_table Function-backed table over the no-arg ten_thousand function
73+
test_same_name_table Schema-disambiguation probe; the data-schema table
7374
tt_pushdown_cols Columns-based: prunes by filter AND time-travels (AT → version arg).
7475
tt_pushdown_fn Function-backed: prunes by filter AND time-travels (AT read at init).
7576
versioned_constraints Table with constraints that evolve across versions

test/sql/integration/table/function_registration.test

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,12 @@ ATTACH 'example' AS example (TYPE vgi, LOCATION '${VGI_TEST_WORKER}');
125125
# producer advertising vgi.cache.ttl, declared in both `main` and `data` so
126126
# its two cache entries prove the result-cache key carries the owning schema.
127127
# Back cache/same_name_schemas.test.
128+
# 2 table-dispatch schema-disambiguation probes: test_same_name_table_scan, a
129+
# one-row producer declared in both `main` and `data`, backing the
130+
# declarative `test_same_name_table` in each schema — proves
131+
# catalog_table_scan_function_get/catalog_table_scan_branches_get resolve
132+
# each schema's own implementation (protocol 1.5.0's schema_name field).
133+
# Back table/same_name_schemas.test.
128134
# 2 global-registration probes (global_table, a producer; global_buffered, a
129135
# table-buffering reducer) — one per function type that surfaces as a table,
130136
# proving a worker-declared global function is registered into the catalog.
@@ -138,7 +144,7 @@ ATTACH 'example' AS example (TYPE vgi, LOCATION '${VGI_TEST_WORKER}');
138144
# re-emits its splits across pages); and four pair splits with another
139145
# feature (split_batch_index, split_partitioned, split_dynamic_filter,
140146
# split_cacheable). Back splits/*.test.
141-
# = 162 portable baseline registrations.
147+
# = 164 portable baseline registrations.
142148
#
143149
# SDK fixture workers may additionally ship the all-or-nothing set of nine
144150
# exchange-cache revalidation/finalization conformance probes listed below.
@@ -160,7 +166,7 @@ WHERE database_name = 'example' AND function_type = 'table'
160166
'cached_slow_sum_all'
161167
);
162168
----
163-
162
169+
164
164170

165171
query I
166172
SELECT CASE
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
# name: test/sql/integration/table/same_name_schemas.test
2+
# description: The plain DECLARATIVE-TABLE member of the schema-disambiguation
3+
# family (see scalar/, table_in_out/ and aggregate/ same_name_schemas.test,
4+
# which probe direct function dispatch, and cache/same_name_schemas.test,
5+
# which probes the result cache). This one probes
6+
# catalog_table_scan_function_get / catalog_table_scan_branches_get — the RPC
7+
# pair that tells a client which function backs a declarative catalog table.
8+
# A function name is unique only within a schema, but a table's own schema
9+
# isn't necessarily where its backing function lives — see
10+
# vgi_table_entry.cpp's GetScanFunctionImpl.
11+
#
12+
# test_same_name_table is registered in BOTH main and data, each backed by
13+
# its OWN same-named scan function (test_same_name_table_scan), each
14+
# emitting a single row tagged with its own schema. This is also the
15+
# end-to-end regression guard for protocol 1.5.0's
16+
# ScanFunctionResult.schema_name / ScanBranch.schema_name: the extension now
17+
# prefers the worker-declared schema over its old table-schema/
18+
# default-schema heuristic. A regression that broke how that field's value
19+
# is threaded through the resolver — not merely a missing value, a value
20+
# mishandled once present — showed up in this session's own investigation
21+
# not as a wrong row here, but as silently disabled filter/projection
22+
# pushdown on a second scan of the same function; see
23+
# cache/filter_pushdown_keys.test and cache/projection_pushdown.test, which
24+
# catch that failure mode directly. This file is the one that would catch a
25+
# worker/client that simply stopped agreeing on which schema's table gets
26+
# which schema's function.
27+
# group: [vgi_integration_table]
28+
29+
require-env VGI_TEST_WORKER
30+
31+
require vgi
32+
33+
require httpfs
34+
35+
statement ok
36+
ATTACH 'example' AS example (TYPE vgi, LOCATION '${VGI_TEST_WORKER}');
37+
38+
# The load-bearing assertion: each schema's declarative table resolves to
39+
# THAT schema's own scan-function implementation, not the other's.
40+
query T
41+
SELECT tag FROM example.main.test_same_name_table;
42+
----
43+
main
44+
45+
query T
46+
SELECT tag FROM example.data.test_same_name_table;
47+
----
48+
data
49+
50+
# Repeat both — proves the resolution is stable across more than one bind
51+
# within the same session (the exact shape of regression this session found:
52+
# correct on the first bind, silently wrong/degraded on a later one).
53+
query T
54+
SELECT tag FROM example.main.test_same_name_table;
55+
----
56+
main
57+
58+
query T
59+
SELECT tag FROM example.data.test_same_name_table;
60+
----
61+
data
62+
63+
# vgi_table_branches() confirms the branches-aware RPC path resolves the same
64+
# function per schema (it surfaces the TABLE's own schema/function_name, not
65+
# the function's resolved schema — this diagnostic predates protocol 1.5.0 —
66+
# but still proves each schema's table names its own scan function, not the
67+
# other's).
68+
query II
69+
SELECT schema_name, function_name FROM vgi_table_branches()
70+
WHERE table_name = 'test_same_name_table'
71+
ORDER BY schema_name;
72+
----
73+
data test_same_name_table_scan
74+
main test_same_name_table_scan

0 commit comments

Comments
 (0)