Close all eight open issues from the first external connector - #16
Merged
Conversation
…s first match
pz.lock.json recorded bare file names, so the target framework and RID
NuGetResolver had already selected were gone by extraction time and
PackageMaterializer re-found each name by a prefix scan. Archive order then
decided which build landed on disk: a net472 assembly on a net10.0 host, an
arm64 native library on x86-64 — both valid files with the right names, so
neither restore nor the lock file looked wrong.
Assets now carry the exact archive path alongside the name they materialize
under (lock schema v2; a v1 lock is diagnosed as its version and regenerated,
since it names no archive paths to upgrade from). Three consequences of the
same root cause close with it:
- transitive native/ assets are flattened into the connector package the way
transitive lib/ already was, because ConnectorLoadContext probes
<lib>/../native and nowhere else;
- two packages providing one file name is refused (PZ0325) instead of one
silently overwriting the other by enumeration order;
- native assets are selected through the RID graph rather than an exact
runtimes/{rid}/ prefix, so a package shipping only linux-x64 is reachable
from linux-musl-x64; a runtimes/ tree matching nothing the host can use is
reported naming the RIDs it does ship.
Closes #8. Closes #9.
The acceptance suite is the contract a connector proves itself against, and subclassing it from outside this repository found four places where it could not express, or did not actually assert, what it claims. - A native-only source could not satisfy the suite at all: every data-plane fact reads through PlanReadAsync, which such a connector is defined to refuse. Those facts now skip for an INativeOnlySource, and a NativeScan_* group asserts what a native reader owes instead — a scan is offered, asking twice is free and gives the same answer, its Mechanism carries no location, a schema is still declared, and the universal path is refused permanently. The suite deliberately does not execute the fragment: that would put DuckDB into every connector's test dependencies. - GateFact() is all-or-nothing and learns nothing about its caller, so a subclass needing to skip one fact had to skip them all. ShouldRun(fact) is the per-fact hook; GateFact() still runs first and still gates everything, so existing subclasses are untouched. - Sink_does_not_retain_engine_owned_batch_instances could not catch what it exists to catch. Reference inequality is satisfied unconditionally by any connector that reads its rows back out of a real destination. The handed batches' buffers are now zeroed after WriteBatchAsync returns — the pool refill the ABI says has already happened — and the committed CONTENT is asserted, so a retaining connector commits recognisably wrong data instead of maybe-crashing. Disposal moves after the assertions on purpose: handing the memory back outright makes a retaining connector's read undefined rather than wrong, and "undefined" is how this passed connectors that were violating the protocol. - Commit_persists_all_written_batches asserted rows in insertion order across the returned list, which ReadCommittedAsync never promises. It sorts now, and the hook says outright that order is not part of its contract. Closes #11.
…scover Both are deliberate engine behaviour with good reasons, and both were discoverable only by reading pz's own source — which is not where a connector author is looking. `PzConnectorException.Message` is trusted TOTALLY: it reaches run_results.json, the NDJSON event stream, and a retry_scheduled reason unredacted. A connector wrapping a third-party client therefore owns redacting it, and owns knowing the shapes that client actually answers in — a real object store's 403 carries the access key id and the signing payload in XML elements, not name=value pairs, so a redactor written against a local emulator can pass its own tests with the redaction removed entirely. `ErrorRedactionContractTests` is the acceptance class for that: it feeds a connector's redactor the shapes real services answer in and requires the credential gone and the diagnosis intact. The same trust carries storage locations into artifacts, which is intended and now said. The schema handed to `BeginWriteAsync` has every field nullable regardless of the source, because DuckDB's Arrow export does not reliably carry NOT NULL through — so a sink's nullability guards are unreachable through pz, and a green `pz run` is not coverage of them. Same for Float, which is not in the v0 type matrix at all. Closes #12. Closes #15.
`base_dir = <projectDir>` was injected by matching the connector NAME against "localfiles" or "sqlite", in six near-identical copies across the verbs and the MCP handlers. Nothing reached a connector loaded from a package, so a relative `root:`/`path:` in its config could only resolve against the process working directory — the same project writing to a different place depending on where `pz` was invoked from, which is the kind of failure found a week later. Who gets an anchor is now declared. A builtin declares itself in ProjectDirectoryAnchor.BuiltinAnchoredConnectors; a package connector declares `"projectDirectoryAnchor": true` in its pz.connector.json. Opt-in both ways: a connector that says nothing receives nothing, so the injected option can never trip a ConnectionConfigSchema's additionalProperties: false — and a connector that opts in declares `base_dir` in that schema, as the builtins already do. Manifests are read straight off disk, never through a loaded assembly: the anchor is applied to the project before the connector registry exists, and pz.connector.json is exactly the file that answers this without an AssemblyLoadContext. Unrestored packages contribute nothing and fail nothing; a broken manifest is left for ConnectorHost to report with its own code and hint. The six copies collapse to one implementation split across the two layers that own its halves — Pz.Core does the injection over PzProject, Pz.PackageManagement reads the manifests. Closes #14.
Nothing reaching a sink distinguished a first run from a retry of the same node, so a sink writing to a store that HAS a transaction or idempotency primitive could not use it — which is what keeps `append` at at-least-once for every such sink. OutputSpec.Attempt carries that identity, additive in the shape of the Keys and MaxTextLengths precedents: every existing `new OutputSpec(...)` compiles and every existing sink behaves exactly as before. Node is pz's content-addressed node id, Run is the run, Ordinal counts attempts from 1 — so a sink that records the identity in the same transaction as the data can skip work a previous attempt already committed. It is stamped only past the native-copy branch: a COPY has no session to carry a progress marker, and TryGetNativeCopy must see the same spec the planner probed with, or a plan could disagree with the run it produced. Scope is stated on the type rather than implied. This makes a within-run retry effectively-once for a capable sink — which is the case duplicates actually come from, a commit that reached the destination and then failed to report back. It does NOT span runs: `pz retry` is a new run with a new Run value, so append stays at-least-once across runs and nothing here should be used to build a cross-run dedupe. (The engine already narrows that window itself: retry carries forward the sinks it observed commit.) Closes #10.
…s out
`partition_by:` was welded to calendar-token path templating: PZ0219 required
tokens in `path:` and treated the option as ONE column whose value substitutes
them. A format that partitions declaratively — Delta, Iceberg, Hive-layout
parquet — records its partition columns in its own metadata and produces the
directories itself, so it has no templated path to route into and could not use
the option at all.
One meaning now: partition_by names the columns an output is partitioned by.
What that produces is the destination's business, and `path:` is what says who
owns the layout — calendar tokens mean pz renders it from one timestamp column
(PathTemplating, unchanged), no tokens mean the destination records its own
partitioning (ColumnPartitionedWrites, new and additive). DagCompiler keeps the
checks it can make connector-agnostically and hands the capability question to
the planner's PZ0314, exactly as the source-side templating refusal already
works.
The type trap behind the same gate goes with it. The presence check was
`.ToString() is { Length: > 0 }`, so a YAML list passed it while meaning
something no downstream code expects: a single column named
System.Collections.Generic.List`1[System.Object]. PartitionColumns is the one
place the option is parsed — a scalar or a sequence, with a coded refusal for
anything else — so no connector re-derives it and none can hit that shape.
Closes #13.
This was referenced Aug 23, 2026
coccor
added a commit
to PipelineZ/pz-connector-deltalake
that referenced
this pull request
Aug 24, 2026
Pre-1.0, breaking changes between connector/pz versions don't need a permanent write-up. Strips the pz 0.2.2-vs-0.3.0 migration narrative (package materializer bug, partition_by gating, PZ0219/PZ0321, PipelineZ/pz#16) from installing.md, limitations.md, troubleshooting.md, compatibility.md, reference/write.md, how-to/partitioned-tables.md, how-to/tune-a-slow-merge.md, and README.md, and corrects a few tables that were still describing the old broken state as current. Docs now just state current requirements (pz 0.3.0+) and how to use the connector.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes all eight open issues — the report set from building
Pz.Connector.DeltaLake, the first external connector against the ABI. Six commits, one per closed concern, each green ondotnet build+dotnet testand onscripts/verify-tool-install.sh.The one real bug
#8 / #9 —
pz.lock.jsonrecorded bare file names, so the target framework and RIDNuGetResolverhad already chosen were gone by extraction time andPackageMaterializerre-found each name by prefix scan. Archive order decided which build landed on disk: anet472assembly on a net10.0 host, anarm64native library on x86-64. Both are valid files with the right names, so neitherpz restorenor the lock file looked wrong.Assets now carry the exact archive path alongside the name they materialize under (lock schema v2; a v1 lock is diagnosed as its version and regenerated, since it names no archive paths to upgrade from). Three consequences of the same root cause close with it:
native/is flattened into the connector package the way transitivelib/already was —ConnectorLoadContextprobes<lib>/../nativeand nowhere else;linux-x64is reachable fromlinux-musl-x64; aruntimes/tree matching nothing the host can use is reported naming the RIDs it does ship.RuntimeIdentifierGraphderives the portable graph from one OS-ancestry table plus one rule and is asserted RID by RID, including that macOS and Windows never reach linux.Design calls made
#13 —
partition_bywas welded to calendar-token paths. The issue asks someone to decide between "make the option mean two things" and "every connector invents its own spelling". Neither: it has one meaning — the columns an output is partitioned by — andpath:is what says who owns the layout. Calendar tokens mean pz renders it from one timestamp column (PathTemplating, unchanged); no tokens mean the destination records its own partitioning (ColumnPartitionedWrites, new and additive).DagCompilerkeeps the checks it can make connector-agnostically and hands the capability question to the planner's PZ0314, exactly as the source-side refusal already works. The.ToString()type trap goes with it:PartitionColumnsis the one place the option is parsed.#14 — the project-directory anchor was matched by connector NAME, in six near-identical copies. It is declared now: a builtin in
ProjectDirectoryAnchor.BuiltinAnchoredConnectors, a package connector via"projectDirectoryAnchor": truein itspz.connector.json. Opt-in both ways, so the injected option can never trip aConnectionConfigSchema'sadditionalProperties: false. Manifests are read straight off disk — the anchor is applied before the connector registry exists. The six copies collapse to one implementation split across the two layers that own its halves.ABI additions (additive only)
#10 —
OutputSpec.Attempttells a sink which attempt at which write it is executing (Node,Run,Ordinal), so a sink whose destination can record a durable progress marker can skip work a previous attempt already committed. Stamped only past the native-copy branch: a COPY has no session to carry a marker, andTryGetNativeCopymust see the same spec the planner probed with.Scope is stated on the type rather than implied: this makes a within-run retry effectively-once — the case duplicates actually come from, a commit that reached the destination and then failed to report back. It does not span runs, and the type says so, so nobody builds a cross-run dedupe on it. Cross-run would need an origin run id threaded through
run_results.jsonand the SQL artifact schema; deliberately not in this PR.#11 — four TestKit gaps. A native-only source could not satisfy the suite at all (every data-plane fact reads through
PlanReadAsync); those facts skip for anINativeOnlySourcenow and aNativeScan_*group asserts what a native reader owes instead.ShouldRun(fact)gives per-fact granularity next to the all-or-nothingGateFact(), which is untouched.Commit_persists_all_written_batchessorts rather than assuming an orderReadCommittedAsyncnever promised.The fourth is the one worth reading:
Sink_does_not_retain_engine_owned_batch_instancescould not catch what it exists to catch — reference inequality is satisfied unconditionally by any connector that reads its rows back out of a real destination. The handed batches' buffers are zeroed afterWriteBatchAsyncreturns (the pool refill the ABI says has already happened) and committed content is asserted. Disposal moves after the assertions on purpose: handing the memory back outright makes a retaining connector's read undefined rather than wrong, and "undefined" is how this passed connectors that were violating the protocol.RetainedBatchDetectionTestsdrives the fact against a deliberately retaining sink and requires the content assertion — not the identity one — to be what fails it.Documentation
#12 / #15 — both are deliberate engine behaviour with good reasons, and both were discoverable only by reading pz's own source. They are stated on the ABI types, where a connector author is actually looking:
PzConnectorException.Messageis trusted totally — verbatim intorun_results.json, the NDJSON stream, and aretry_scheduledreason. A connector wrapping a third-party client owns redacting it, and owns knowing the shapes that client answers in.ErrorRedactionContractTestsis the new acceptance class for that: it feeds a redactor an S3 XML 403, an AzureAuthorizationFailure, a connection string, and aname=valuepair, and requires the credential gone and the diagnosis intact. Its self-test proves aname=value-only redactor — the one from the report — fails it.BeginWriteAsynchas every field nullable regardless of source, so a sink's nullability guards are unreachable through pz and a greenpz runis not coverage of them. Same forFloat, absent from the v0 type matrix.Site pages that need a matching PR in
pz-sitepz.lock.jsonis schema v2 and records per-asset archive paths.PzConnectorException.Messagetrust boundary; the all-nullable write schema;pz.connector.json'sprojectDirectoryAnchor;ConnectorCapabilities.ColumnPartitionedWrites;OutputSpec.Attempt;ErrorRedactionContractTests.connections.ymlreference:partition_by:takes a column name or a list, and no longer requires calendar tokens inpath:.append.Verification
dotnet build Pz.slnx -c Release— 0 warnings.PZ_TESTS_OFFLINE=1 dotnet test Pz.slnx -c Release— 0 failures across all 21 test projects, exit 0 (docker suites ran; nothing skipped for docker on this machine).scripts/verify-tool-install.sh— PASS: pack → tool install → offlineinit→ offlinerun --all.New error code: PZ0325 (
PackageAssetCollision). New capability:ColumnPartitionedWrites.One behaviour change worth flagging in review
Two resolved packages providing the same
lib/ornative/file name is now a hard restore failure (PZ0325) rather than a silent overwrite. That is what #8 asks for and it matches the no-silent-failures rule, but it can turn a project that "worked" into one that reports a real ambiguity.