Skip to content

Commit f7eb2d1

Browse files
rustyconoverclaude
andcommitted
feat(optimizer): enforce required_field_filter_paths on VGI Tables
Adds VgiRequiredFiltersOptimizer (post-optimize OptimizerExtension) that walks every LogicalGet and rejects scans missing any path the Table declared in TableInfo.required_field_filter_paths. The error fires at bind/optimize time before any worker / S3 byte is read: Binder Error: Table 'overture.places.place' requires WHERE filters on: bbox.xmin, bbox.xmax, bbox.ymin, bbox.ymax. Missing: bbox.xmax, bbox.ymin, bbox.ymax. ... Prefix-aware: a filter on a parent struct (e.g. WHERE bbox = small) satisfies all bbox.* requirements, because the wider filter is at least as constraining as the four-corner conjunction. Native scan delegation via ScanFunctionResult(function_name= "read_parquet", ...): VgiTableEntry::GetScanFunctionImpl recognises SYSTEM_CATALOG function names, eagerly binds the native function, validates that declared catalog columns match the bind output by name+position (catches Hive-partition column mismatches like the Overture theme/type columns at attach time), and stows the bound function inside a VgiNativeDelegationMarkerBindData placeholder. VgiRequiredFiltersOptimizer then runs its filter-presence check against the marker's catalog metadata and swaps the marker out for the real native bind — post-pass the LogicalGet looks exactly like what DuckDB would produce for a direct read_parquet call, so every native callback (scan/init/statistics/dynamic_to_string/future MultiFile additions) just works. The marker function vgi_native_delegation_marker is registered with a throwing deserialize callback: it names the function and explains the optimizer-ordering invariant if it's ever serialized (a LogicalOperator::Copy() round-trips through serialize/deserialize and looks the function up by name in the system catalog). WalkTableFilter (the path-collection helper) mirrors the existing serializer at vgi_table_function_impl.cpp:714 — OPTIONAL_FILTER subtrees containing a DynamicFilter are skipped so Top-N machinery artifacts can't spuriously satisfy required_field_filter_paths the user never wrote. VgiContainsDynamicFilter is promoted from anonymous-namespace static to external linkage in duckdb::vgi. Wire-additive on TableInfo (see vgi_catalog_api.cpp ParseTableInfo); backward-compatible with older workers. extension_config.cmake: hard-code haybarn-httpfs until the CMake- side HAYBARN_VERSION_STRING propagation is sorted (the C++ -D define is on the compile line but no set(...) reaches this scope, so the stock httpfs was silently winning the local make release path). Sqllogictests under test/sql/integration/table/required_field_- filter_paths_*.test exercise basic/struct/nested/prefix/disjunction- null/above-get/complex scenarios; comments.test + function_- registration.test counts adjusted for the new rff_* fixtures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 46310c8 commit f7eb2d1

17 files changed

Lines changed: 1072 additions & 18 deletions

extension_config.cmake

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,22 @@ duckdb_extension_load(vgi
1010
# engine fork defines HAYBARN_VERSION_STRING (a top-level CMake var, set before
1111
# extension configs are included); stock DuckDB never does. Build against the
1212
# matching httpfs in each case — they have genuinely different sources.
13-
if(DEFINED HAYBARN_VERSION_STRING)
14-
set(HTTPFS_GIT_URL https://github.com/Query-farm-haybarn/haybarn-httpfs)
15-
set(HTTPFS_GIT_TAG 1f23e5bd8f7a50253c08b00bb7a88ecfa15862df)
16-
else()
17-
set(HTTPFS_GIT_URL https://github.com/duckdb/duckdb-httpfs)
18-
set(HTTPFS_GIT_TAG 52afb4204a3238d6ee132e83340f8d68c40ee91c)
19-
endif()
13+
#
14+
# TEMPORARILY hard-coded to the haybarn fork: the CMake-side
15+
# `if(DEFINED HAYBARN_VERSION_STRING)` check wasn't firing on the local
16+
# `make release` path here (the C++ `-DHAYBARN_VERSION_STRING=...` preprocessor
17+
# define is set on the compile command line but no `set(...)` CMake variable
18+
# propagates to this scope), so the stock-DuckDB httpfs was being selected.
19+
# Force the haybarn fork until the CMake propagation is sorted out.
20+
set(HTTPFS_GIT_URL https://github.com/Query-farm-haybarn/haybarn-httpfs)
21+
set(HTTPFS_GIT_TAG 1f23e5bd8f7a50253c08b00bb7a88ecfa15862df)
22+
# if(DEFINED HAYBARN_VERSION_STRING)
23+
# set(HTTPFS_GIT_URL https://github.com/Query-farm-haybarn/haybarn-httpfs)
24+
# set(HTTPFS_GIT_TAG 1f23e5bd8f7a50253c08b00bb7a88ecfa15862df)
25+
# else()
26+
# set(HTTPFS_GIT_URL https://github.com/duckdb/duckdb-httpfs)
27+
# set(HTTPFS_GIT_TAG 52afb4204a3238d6ee132e83340f8d68c40ee91c)
28+
# endif()
2029

2130
# DuckDB 1.5.3's httpfs (52afb42) is curl-based. On the MinGW toolchain
2231
# (x64-mingw-static, rtools GCC) it fails to link against static curl with

src/generated/vgi_protocol_schemas.hpp

Lines changed: 2 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: b6d03efdf957
5+
// Content hash: 15ff8ede31b1
66
//
77
// To regenerate:
88
// uv run --project ~/Development/vgi-python vgi-gen-cpp-schemas \
@@ -69,6 +69,7 @@ inline const std::shared_ptr<arrow::Schema> &TableInfoSchema() {
6969
arrow::field("cardinality_max", arrow::int64(), /*nullable=*/false),
7070
arrow::field("column_statistics", arrow::binary(), /*nullable=*/false),
7171
arrow::field("bind_result", arrow::binary(), /*nullable=*/false),
72+
arrow::field("required_field_filter_paths", arrow::list(arrow::utf8()), /*nullable=*/false),
7273
});
7374
return schema;
7475
}

src/include/storage/vgi_table_entry.hpp

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,67 @@
99

1010
#include "duckdb/catalog/catalog_entry/table_catalog_entry.hpp"
1111
#include "duckdb/catalog/entry_lookup_info.hpp"
12+
#include "duckdb/function/table_function.hpp"
1213
#include "duckdb/parser/parsed_data/create_table_info.hpp"
14+
#include "duckdb/planner/operator/logical_get.hpp"
1315
#include "duckdb/storage/statistics/base_statistics.hpp"
1416
#include "vgi_catalog_api.hpp"
1517

1618
namespace duckdb {
1719

1820
class VgiCatalog;
1921
class VgiSchemaEntry;
22+
class VgiTableEntry;
23+
24+
// VgiNativeDelegationMarkerBindData — single-branch sibling of
25+
// VgiMultiBranchMarkerBindData. Carried by the placeholder TableFunction
26+
// returned from VgiTableEntry::GetScanFunctionImpl when the worker's
27+
// ScanFunctionResult names a SYSTEM_CATALOG function (read_parquet, etc.).
28+
// VgiRequiredFiltersOptimizer (vgi_extension.cpp) detects this marker via
29+
// `dynamic_cast<VgiNativeDelegationMarkerBindData *>(get.bind_data.get())`,
30+
// enforces `table.required_field_filter_paths`, and rewrites the LogicalGet
31+
// in place to point at the bound native TableFunction. See the comment
32+
// block at the struct definition in vgi_table_entry.cpp for full lifecycle.
33+
struct VgiNativeDelegationMarkerBindData : public duckdb::TableFunctionData {
34+
// Owning VgiTableEntry — the rewriter reads
35+
// `required_field_filter_paths` from this to know which paths must appear
36+
// in `LogicalGet::table_filters`.
37+
reference<VgiTableEntry> table;
38+
39+
// Pre-bound native TableFunction + bind_data. GetScanFunctionImpl binds
40+
// eagerly (so we can populate the LogicalGet's returned_types/names
41+
// faithfully), and the rewriter just transfers ownership of these into
42+
// the LogicalGet during the in-place swap. No re-binding in the optimizer.
43+
TableFunction native_tf;
44+
mutable unique_ptr<duckdb::FunctionData> native_bind;
45+
std::vector<LogicalType> native_return_types;
46+
std::vector<std::string> native_return_names;
47+
virtual_column_map_t native_virtual_columns;
48+
49+
// Originating ScanFunctionResult — kept for diagnostics + logging.
50+
vgi::VgiScanFunctionResult scan_result;
51+
std::string worker_path;
52+
53+
VgiNativeDelegationMarkerBindData(VgiTableEntry &table_p, TableFunction native_tf_p,
54+
unique_ptr<duckdb::FunctionData> native_bind_p,
55+
std::vector<LogicalType> native_return_types_p,
56+
std::vector<std::string> native_return_names_p,
57+
virtual_column_map_t native_virtual_columns_p,
58+
vgi::VgiScanFunctionResult scan_result_p,
59+
std::string worker_path_p)
60+
: table(table_p), native_tf(std::move(native_tf_p)),
61+
native_bind(std::move(native_bind_p)),
62+
native_return_types(std::move(native_return_types_p)),
63+
native_return_names(std::move(native_return_names_p)),
64+
native_virtual_columns(std::move(native_virtual_columns_p)),
65+
scan_result(std::move(scan_result_p)), worker_path(std::move(worker_path_p)) {
66+
}
67+
};
68+
69+
// Construct the native-delegation marker. Its execute callback throws
70+
// InternalException — reaching it means VgiRequiredFiltersOptimizer didn't
71+
// run, which IS a bug.
72+
TableFunction MakeNativeDelegationMarkerFunction();
2073

2174
class VgiTableEntry : public TableCatalogEntry {
2275
public:

src/include/vgi_catalog_api.hpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -458,6 +458,19 @@ struct VgiTableInfo {
458458
// declarative path (which is restricted to `@bind_fixed_schema`-decorated
459459
// functions whose bind output is a pure function of `cls.FIXED_SCHEMA`).
460460
std::optional<std::vector<uint8_t>> bind_result;
461+
462+
// Dotted-path column references that the optimizer extension must verify
463+
// appear in any scan's WHERE expression. Top-level column names
464+
// (`"country"`) or struct subfields (`"bbox.xmin"`, `"nested.outer.inner"`).
465+
// Empty means no enforcement — the zero-cost fast path for every existing
466+
// table.
467+
//
468+
// Satisfaction is prefix-based: a present filter on a shorter dotted path
469+
// satisfies any required path it's a prefix of. A whole-struct filter on
470+
// `bbox` therefore satisfies every required `"bbox.*"` path.
471+
// `VgiRequiredFiltersOptimizer` consults this list at post-optimize time
472+
// and throws `BinderException` listing any unsatisfied paths.
473+
std::vector<std::string> required_field_filter_paths;
461474
};
462475

463476
// View metadata from the worker

src/include/vgi_table_function_impl.hpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -584,6 +584,14 @@ SerializedFilters VgiSerializeFilters(ClientContext &context, const vector<colum
584584
const vector<string> &column_names, const string &worker_path,
585585
const string &rowid_column_name = "");
586586

587+
//! Returns true if any descendant of ``filter`` is a DynamicFilter (Top-N
588+
//! tick-time bound). Consumers that walk TableFilter trees for *static*
589+
//! information (presence-of-filter checks, serialization of constants, etc.)
590+
//! should skip the entire OptionalFilter subtree when this returns true:
591+
//! the DynamicFilter has no value at init time, and any partial walk yields
592+
//! a stricter view than the OptionalFilter actually constrains.
593+
bool VgiContainsDynamicFilter(const TableFilter &filter);
594+
587595
//! Expression pushdown callback: checks if the expression tree only uses functions the worker supports
588596
bool VgiPushdownExpression(ClientContext &context, const LogicalGet &get, Expression &expr);
589597

src/storage/vgi_table_entry.cpp

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "duckdb/function/table_function.hpp"
99
#include "duckdb/main/client_context.hpp"
1010
#include "duckdb/main/extension_helper.hpp"
11+
#include "duckdb/parser/tableref/table_function_ref.hpp"
1112
#include "duckdb/planner/tableref/bound_at_clause.hpp"
1213
#include "duckdb/storage/table_storage_info.hpp"
1314

@@ -21,6 +22,36 @@
2122

2223
namespace duckdb {
2324

25+
// VgiNativeDelegationMarkerBindData is declared in storage/vgi_table_entry.hpp
26+
// so VgiRequiredFiltersOptimizer (vgi_extension.cpp) can dynamic_cast against
27+
// it. See the header for full docstring and lifecycle.
28+
29+
namespace {
30+
31+
// Marker placeholder TableFunction — never executed; the optimizer
32+
// extension must replace it. Mirrors MakeMultiBranchMarkerFunction in
33+
// vgi_multi_scan_rewriter.cpp.
34+
void NativeDelegationMarkerExecute(ClientContext &, TableFunctionInput &, DataChunk &) {
35+
throw InternalException(
36+
"VgiRequiredFiltersOptimizer did not fire — native-delegation placeholder "
37+
"reached execution. Check that the optimizer extension is registered and "
38+
"that no other pass dropped the marker. This is a bug — please report it.");
39+
}
40+
41+
} // namespace
42+
43+
TableFunction MakeNativeDelegationMarkerFunction() {
44+
TableFunction fn("vgi_native_delegation_marker", {}, NativeDelegationMarkerExecute);
45+
// No bind callback — bind_data is supplied externally by GetScanFunctionImpl.
46+
// No init_global / init_local — the marker should never be executed.
47+
// filter_pushdown=true so DuckDB's FilterPushdown still installs filters
48+
// on this LogicalGet's table_filters; the rewriter then hands them off to
49+
// the real native function on the rewritten LogicalGet.
50+
fn.filter_pushdown = true;
51+
fn.projection_pushdown = true;
52+
return fn;
53+
}
54+
2455
VgiTableEntry::VgiTableEntry(Catalog &catalog, SchemaCatalogEntry &schema, CreateTableInfo &info,
2556
const vgi::VgiTableInfo &table_info)
2657
: TableCatalogEntry(catalog, schema, info), table_info_(table_info), catalog_(catalog) {
@@ -482,6 +513,111 @@ TableFunction VgiTableEntry::GetScanFunctionImpl(ClientContext &context, unique_
482513
context, default_schema, scan_result.function_name, OnEntryNotFound::RETURN_NULL);
483514
}
484515
}
516+
bool from_system_catalog = false;
517+
if (!func_entry) {
518+
// Last-resort fallback to the system catalog for built-in DuckDB
519+
// table functions like `read_parquet` or `iceberg_scan` that workers
520+
// declare via ScanFunctionResult. The multi-branch rewriter
521+
// (vgi_multi_scan_rewriter.cpp:203) already does this fallback —
522+
// mirror it for the single-branch path so workers can delegate scans
523+
// to native DuckDB functions without going through a UNION ALL.
524+
EntryLookupInfo lookup(CatalogType::TABLE_FUNCTION_ENTRY, scan_result.function_name);
525+
auto sys_entry = Catalog::GetEntry(context, SYSTEM_CATALOG, DEFAULT_SCHEMA, lookup,
526+
OnEntryNotFound::RETURN_NULL);
527+
if (sys_entry) {
528+
func_entry = &sys_entry->Cast<TableFunctionCatalogEntry>();
529+
from_system_catalog = true;
530+
}
531+
}
532+
533+
if (from_system_catalog && func_entry) {
534+
// Native delegation: bind the system function eagerly here, then return
535+
// a marker carrying the bound function + bind_data + return shapes.
536+
// VgiRequiredFiltersOptimizer (vgi_extension.cpp) enforces this table's
537+
// `required_field_filter_paths` against the LogicalGet's table_filters,
538+
// then swaps `function` / `bind_data` / `returned_types` / `names` in
539+
// place to the stashed native ones. Subsequent passes see a vanilla
540+
// native scan. Matches VgiMultiScanRewriter's per-arm binding shape
541+
// (vgi_multi_scan_rewriter.cpp:219-247) but for the single-branch path.
542+
vector<LogicalType> arg_types;
543+
arg_types.reserve(scan_result.positional_arguments.size());
544+
for (const auto &v : scan_result.positional_arguments) {
545+
arg_types.push_back(v.type());
546+
}
547+
TableFunction native_tf =
548+
func_entry->functions.GetFunctionByArguments(context, arg_types);
549+
vector<Value> parameters(scan_result.positional_arguments.begin(),
550+
scan_result.positional_arguments.end());
551+
named_parameter_map_t named_parameters;
552+
for (auto &kv : scan_result.named_arguments) {
553+
named_parameters.emplace(kv.first, kv.second);
554+
}
555+
vector<LogicalType> input_table_types;
556+
vector<string> input_table_names;
557+
TableFunctionRef ref;
558+
TableFunctionBindInput bind_input(parameters, named_parameters, input_table_types,
559+
input_table_names, native_tf.function_info.get(),
560+
nullptr, native_tf, ref);
561+
vector<LogicalType> return_types;
562+
vector<string> return_names;
563+
auto native_bind = native_tf.bind(context, bind_input, return_types, return_names);
564+
virtual_column_map_t native_virtual_columns;
565+
if (native_tf.get_virtual_columns) {
566+
native_virtual_columns = native_tf.get_virtual_columns(context, native_bind.get());
567+
}
568+
569+
// Validate the catalog's declared columns match the native bind's
570+
// output by position+name. The LogicalGet that DuckDB constructs uses
571+
// the catalog's column list for FilterPushdown's column_ids /
572+
// table_filters keys; if the native function emits a different shape,
573+
// those indices mis-resolve once VgiRequiredFiltersOptimizer rewrites
574+
// the marker. Two common causes:
575+
// - The worker's pa.Schema source omits Hive-partition columns that
576+
// the native bind appends (read_parquet on `theme=…/type=…/*`).
577+
// - The worker introspected against a different release than what
578+
// the URL points at.
579+
// Either way the right move is to fail loudly here so the worker
580+
// author sees the mismatch immediately instead of silent column
581+
// misrouting at scan time.
582+
{
583+
const auto &decl_columns = GetColumns();
584+
const auto decl_count = decl_columns.LogicalColumnCount();
585+
if (decl_count != return_names.size()) {
586+
throw BinderException(
587+
"VGI native delegation for '%s.%s.%s' (function '%s'): catalog declares "
588+
"%llu column(s) but the native bind returned %llu. The catalog's columns "
589+
"must match exactly what the native function emits at scan time (positions "
590+
"+ names). Common cause: Hive-partition columns that read_parquet appends "
591+
"but the worker's schema source omitted.",
592+
catalog_.GetName(), ParentSchema().name, name, scan_result.function_name,
593+
static_cast<unsigned long long>(decl_count),
594+
static_cast<unsigned long long>(return_names.size()));
595+
}
596+
for (idx_t i = 0; i < decl_count; ++i) {
597+
const auto &decl_name = decl_columns.GetColumn(LogicalIndex(i)).Name();
598+
if (decl_name != return_names[i]) {
599+
throw BinderException(
600+
"VGI native delegation for '%s.%s.%s' (function '%s'): catalog "
601+
"declared column %llu as '%s' but the native bind returned '%s'. "
602+
"Names must match by position.",
603+
catalog_.GetName(), ParentSchema().name, name, scan_result.function_name,
604+
static_cast<unsigned long long>(i), decl_name, return_names[i]);
605+
}
606+
}
607+
}
608+
609+
auto function_name_log = scan_result.function_name;
610+
bind_data = make_uniq<VgiNativeDelegationMarkerBindData>(
611+
*this, std::move(native_tf), std::move(native_bind), std::move(return_types),
612+
std::move(return_names), std::move(native_virtual_columns), std::move(scan_result),
613+
attach_params->worker_path());
614+
615+
VGI_LOG(context, "vgi.scan_function.native_delegation_marker",
616+
{{"schema", ParentSchema().name},
617+
{"table", name},
618+
{"function", function_name_log}});
619+
return MakeNativeDelegationMarkerFunction();
620+
}
485621
if (func_entry) {
486622
for (auto &tf : func_entry->functions.functions) {
487623
has_projection_pushdown = has_projection_pushdown || tf.projection_pushdown;

src/vgi_catalog_api.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1903,6 +1903,13 @@ VgiTableInfo ParseTableInfo(ClientContext &context, const std::shared_ptr<arrow:
19031903
bind_bytes.empty() ? std::nullopt : std::make_optional(std::move(bind_bytes));
19041904
}
19051905

1906+
// Parse required_field_filter_paths (optional, backward-compatible —
1907+
// missing column or empty list means no enforcement). The optimizer
1908+
// extension VgiRequiredFiltersOptimizer reads these from the cached
1909+
// VgiTableInfo at bind/optimize time.
1910+
info.required_field_filter_paths =
1911+
row["required_field_filter_paths"].value_or(std::vector<std::string>{});
1912+
19061913
// Validate: UPDATE/DELETE require a row ID column
19071914
if ((info.supports_update || info.supports_delete) && info.row_id_column < 0) {
19081915
throw InvalidInputException(

0 commit comments

Comments
 (0)