Skip to content

Commit f0e746d

Browse files
rustyconoverclaude
andcommitted
Fix review issues from secrets system overhaul
- Fix scalar 0-column/0-row validation: validate output against synthetic batch instead of skipping validation when inject_row=True - Remove dead _validate_required_secrets no-op method and call site - Deduplicate secret batch parsing: add SecretsAccessor.to_dict(), replace all 6 _batch_to_secret_dict() call sites, delete the old function - Use SecretLookupEntry (now extends ArrowSerializableDataclass) instead of untyped dicts for required_secrets throughout metadata and catalog - Eliminate CatalogSecretRequirement which duplicated SecretLookupEntry - Remove backward-compat isinstance(entry, str) branch in resolve_metadata - Add SecretLookupEntry to __all__ in arguments.py - Add type-ignore explanation comments for PyArrow field metadata limitation - Fix pre-existing mypy errors (boto3/aiohttp import-not-found suppressions) - Add tests: SecretsAccessor (get, scoped, required, to_dict), BindResponse.secret_scope_request round-trip, TState validation, SecretTypeSpec serialization round-trip Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 542c64a commit f0e746d

23 files changed

Lines changed: 1054 additions & 142 deletions

‎CLAUDE.md‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,49 @@ uv run ruff check --fix . && uv run ruff format .
3737

3838
When making changes, we don't need to worry about backward compatibility, make the changes and change the import references.
3939

40+
## Integration Testing
41+
42+
Integration tests live in the `vgi` C++ repo (sibling directory) at `test/sql/integration/`.
43+
They run DuckDB's sqllogictest framework against a real VGI worker. Run from the `vgi` directory.
44+
45+
**Subprocess transport** (worker spawned as a child process):
46+
```bash
47+
cd ~/Development/vgi
48+
VGI_TEST_WORKER="uv run --project ~/Development/vgi-python vgi-example-worker" \
49+
./build/release/test/unittest "test/sql/integration/*"
50+
```
51+
52+
**HTTP transport** (worker as an HTTP server):
53+
```bash
54+
cd ~/Development/vgi
55+
./test/run_http_integration.sh "test/sql/integration/*"
56+
```
57+
58+
The HTTP script (`test/run_http_integration.sh`) starts the HTTP server, waits for it to be
59+
ready, runs the tests, and cleans up. Always use this script instead of manually managing
60+
the server. Server logs are at `/tmp/vgi-http-test-server.log`.
61+
62+
**Filter by test subset:**
63+
```bash
64+
./test/run_http_integration.sh "test/sql/integration/secret/*"
65+
./test/run_http_integration.sh "test/sql/integration/table/countdown*"
66+
```
67+
68+
**Running ad-hoc SQL against DuckDB CLI:**
69+
Use `-f` to supply SQL files to DuckDB, not stdin redirection. Do not redirect stderr to stdout.
70+
```bash
71+
# Correct:
72+
~/Development/vgi/build/debug/duckdb -f /tmp/my_test.sql
73+
74+
# Wrong:
75+
~/Development/vgi/build/debug/duckdb < /tmp/my_test.sql 2>&1
76+
```
77+
78+
**Known HTTP limitations** (4 tests fail, not regressions):
79+
- `logging_generator.test` — inline log streaming not supported over HTTP
80+
- `partitioned_sequence.test` — partition-local state not preserved across HTTP exchanges
81+
- `buffer_input/sizes.test`, `buffer_input/scale.test_slow` — input buffering semantics differ
82+
4083
## Project Overview
4184

4285
VGI (Vector Gateway Interface) provides an Apache Arrow-based protocol for connecting DuckDB to external programs. It enables user-defined functions to run in separate processes, communicating via stdin/stdout using Arrow IPC streaming.

‎tests/catalog/test_serialization.py‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -766,7 +766,7 @@ class TestArrowSchemaCorrectness:
766766
def test_catalog_attach_result_schema(self) -> None:
767767
"""Verify CatalogAttachResult Arrow schema."""
768768
schema = CatalogAttachResult.ARROW_SCHEMA
769-
assert len(schema) == 8
769+
assert len(schema) == 9
770770
assert schema.field("attach_id").type == pa.binary()
771771
assert schema.field("supports_transactions").type == pa.bool_()
772772
assert schema.field("supports_time_travel").type == pa.bool_()
@@ -775,6 +775,7 @@ def test_catalog_attach_result_schema(self) -> None:
775775
assert schema.field("attach_id_required").type == pa.bool_()
776776
assert schema.field("default_schema").type == pa.string()
777777
assert schema.field("settings").type == pa.list_(pa.binary())
778+
assert schema.field("secret_types").type == pa.list_(pa.binary())
778779

779780
def test_schema_info_schema(self) -> None:
780781
"""Verify SchemaInfo Arrow schema."""
@@ -816,3 +817,33 @@ def test_scan_function_result_schema(self) -> None:
816817
assert schema.field("function_name").type == pa.string()
817818
assert schema.field("arguments").type == pa.binary()
818819
assert schema.field("required_extensions").type == pa.list_(pa.string())
820+
821+
822+
class TestSecretTypeSpecSerialization:
823+
"""Test SecretTypeSpec serialization round-trip."""
824+
825+
def test_round_trip(self) -> None:
826+
"""Test basic serialization and deserialization."""
827+
from vgi.catalog.secret_type import SecretTypeSpec
828+
829+
original = SecretTypeSpec(
830+
name="vgi_example",
831+
description="Example VGI secret for testing",
832+
schema=pa.schema(
833+
[
834+
pa.field("secret_string", pa.string(), metadata={"redact": "true"}),
835+
pa.field("api_key", pa.string(), metadata={"redact": "true"}),
836+
pa.field("port", pa.int32()),
837+
] # type: ignore[arg-type] # PyArrow field metadata typing limitation
838+
),
839+
)
840+
serialized = original.serialize()
841+
batch, _ = deserialize_record_batch(serialized)
842+
restored = SecretTypeSpec.deserialize(batch)
843+
844+
assert restored.name == original.name
845+
assert restored.description == original.description
846+
assert restored.schema.names == original.schema.names
847+
# Verify field metadata survives round-trip
848+
assert restored.schema.field("secret_string").metadata == {b"redact": b"true"}
849+
assert restored.schema.field("port").metadata is None

‎tests/scalar/test_return_secret_value_function.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ def test_return_secret_value_basic(self, example_worker: str) -> None:
2525
client.scalar_function(
2626
function_name="return_secret_value",
2727
input=iter([batch]),
28-
secrets={"vgi_example_secret": secret_value},
28+
secrets={"vgi_example": secret_value},
2929
)
3030
)
3131

‎tests/test_http_s3_offload_input.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ def test_http_input_upload_url_then_external_location_scalar_exchange(compressio
113113
pytest.importorskip("vgi_rpc.s3")
114114

115115
if compression == "zstd":
116-
import aiohttp.http_parser as http_parser
116+
import aiohttp.http_parser as http_parser # type: ignore[import-not-found]
117117

118118
if not bool(getattr(http_parser, "HAS_ZSTD", False)):
119119
pytest.skip("zstd input test requires aiohttp zstd decode support (backports.zstd)")

‎tests/test_http_s3_offload_output.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ def test_http_output_offload_random_bytes_large_response(compression: str, expec
115115
pytest.importorskip("vgi_rpc.http")
116116
pytest.importorskip("vgi_rpc.s3")
117117
if compression == "zstd":
118-
import aiohttp.http_parser as http_parser
118+
import aiohttp.http_parser as http_parser # type: ignore[import-not-found]
119119

120120
if not bool(getattr(http_parser, "HAS_ZSTD", False)):
121121
pytest.skip("zstd externalization test requires aiohttp zstd decode support (backports.zstd)")

0 commit comments

Comments
 (0)