Repository navigation
DatasetReader::Scan honours options protowire-go's Scan drops (skip_validate, skip_post_decode, discard_unknown, on_secret_field, validator) #166
Description
DatasetReader::Scan binds each row with the reader's whole UnmarshalOptions. protowire-go's Scan builds fresh options that carry only the limits and the TypeResolver. So a cpp caller who sets any of five options gets a different bind than a Go caller with the same options. Found while fixing #164. That PR matched Go only on max_message_size, which the issue named.
What each side does (from the code, not measured)
Go, encoding/pxf/dataset_bind.go at protowire-go 903b4a7 (v1.16.1):
o := UnmarshalOptions{
MaxNestingDepth: tr.lim.maxDepth,
MaxNumericLiteralDigits: tr.lim.maxDigits,
MaxBytesLiteralLength: tr.lim.maxBytesLiteral,
MaxRepeatedCount: tr.lim.maxRepeated,
MaxBigFloatExponent: tr.lim.maxBigFloatExp,
TypeResolver: tr.resolver,
}cpp, src/pxf/dataset_reader.cc Scan: UnmarshalOptions opts = opts_;, then skip_validate = true (after its own once-per-type check) and max_message_size = 0.
| Option | Go Scan |
cpp Scan |
|---|---|---|
skip_validate |
ignored: the schema is always checked (memoized) | honoured, and documented in dataset_reader.h ("opts.skip_validate turns off Scan's schema check") |
skip_post_decode |
ignored: a (pxf.required) empty cell is refused, (pxf.default) applied |
honoured, so the post-decode pass would be skipped |
discard_unknown |
ignored | honoured, e.g. in a block cell { unknown = 1 } |
on_secret_field |
ignored: a pxf.Secret cell goes into the message |
honoured: the hook gets it |
validator / report |
ignored | honoured |
These rows come from reading the code. The table has not been checked by running both ports on the same input. That should be the first step of the fix (a probe like tools/dataset_stream_sweep).
Options
- Match Go (the default: every port behaves as protowire-go does):
Scanbuilds its options as Go does. cpp loses documented behaviour, at least theskip_validateline indataset_reader.h. Reversible by a revert. It changes what callers are promised. - Keep cpp's behaviour and raise it for protowire-go: if passing the options through is the better contract, Go changes first and cpp stays.
- Split: match Go where cpp's behaviour cannot have been relied on, and decide the rest one option at a time.
Decision needed
This is the owner's call. Option 1 changes a documented contract, and option 2 is a change to Go. No recommendation yet: the table above has to be measured first, and whether on_secret_field should reach Scan is a design question in itself.
Done when
- The table above is measured against protowire-go for each option.
Scanbehaves as decided, with a test per option.dataset_reader.hsays which optionsScanuses.
Activity
bfrancojr commentedon Nov 7, 2026
Measured: the table holds on every row
The same 12 cases ran through each port's
Scan, each option off and on. Go was protowire-go 903b4a7 (v1.16.1). cpp was PR #167's head 50953c5. Row typedataset.v1.Eventcomes fromtestdata/dataset/paths.proto. Theskip_validatecases use a probe type with a field namednull, which the schema check refuses.ScanScanreq.nmissing2:1: required field "req.n" is absentskip_post_decodereq.dreq.d="dflt"skip_post_decodereq.d="dflt"req.d=""2:1: column desc: unknown field "nope" in dataset.v1.Descdiscard_unknownpxf.Secretcellpw.value="s3cret"on_secret_fieldpw.value="s3cret"pw=s3cret,pw.value=""2:1: check: id: denied (probe.deny)nullPXF schema violations: … uses PXF-reserved name "null"skip_validateSo Go's
Scanignores all five options, and cpp's honours all five.Go is not consistent with itself
Go's options-taking
BindRow, the materializing path, does honour them:func (o UnmarshalOptions) BindRow(...) { return o.bindRow(msg, columns, row, false) }. So in Go today, the same row with the same options binds differently depending on whether it came fromParse+BindRowor fromDatasetReader.Scan.The history reads as incidental. In d1d30ac (protowire-go#152, value cells), the reader started keeping only
lim limitsinstead of the options.Scanrebuilt its options from those limits. Fields came back one at a time when a gap was found:TypeResolverin f1d98b8 (#173),MaxBigFloatExponentin 56afac9 (#185). I looked for anything that would say the dropping is intended and found nothing. No Go test runsScanwith any of the five options, and the spec says nothing about which options apply to streamed rows.Recommendation: option 2. Go changes, and cpp keeps its behaviour
Go's reader would keep the caller's options, and
Scanwould bind with them exceptMaxMessageSize, as it does for the limits today. ThenScanandBindRowagree in Go, and cpp already matches. cpp then only needs the tests and the header line in this issue's done-when.NewDatasetReadergets them silently ignored.Scandropping these is intended, option 1 is right instead, and Go'sBindRowwould be the one to change. A protowire-go maintainer's statement or a design note saying so would settle that. I found none.on_secret_fieldshould reach streamed rows. cpp says yes. Under option 2, Go would say yes too.Waiting on your decision. I've changed no code for this issue.
bfrancojr commentedon Nov 7, 2026
Decided (owner, 2026-11-07)
Scanbinds with them, exceptMaxMessageSize. cpp keeps its behaviour and gets a test per option, plus a header line saying which optionsScanuses.on_secret_fieldfires for streamed rows, as it does forParse+BindRow.pxf: a row's validator and schema errors carry no row position (#166) (…
release: cut v2.1.0
release: cut v2.1.0 (#175)