From 9e34f3827f689e75db553eb4e3537f7826d3a1c8 Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Sat, 7 Nov 2026 16:58:57 -0800 Subject: [PATCH] pxf: keep list, block and precision in dataset cells A row's cells as Python saw them lost part of the row: a list or block cell came back as ("unknown", None), and a number's precision suffix was dropped (1.5p80 -> ("float", "1.5")). So a row read with next_or_none and passed to bind_row did not bind as scan binds it, and Python's cells held less than protowire-go's DatasetRow, whose cells are the full parse tree (ListVal.Elements, BlockVal.Entries, IntVal/FloatVal.Prec). The owner chose option 2 on #47: lists and blocks become structured cells, ("list", [cell, ...]) and ("block", [entry, ...]), and the suffix stays in the raw text. An entry is (kind, key, value, quoted), one per kind of cpp's EntryPtr: Assignment, MapEntry, Block and TypeEntry, with the key's quoting kept, since it tells the string map key "1" from the integer key 1. The conversion both ways is now recursive. The suffix is split back off at its 'p', which is unambiguous since PXF numbers are decimal (cpp's lexer: LexNumber has no hex form, LexPrecision takes 'p' and digits). Fixes #47 --- CHANGELOG.md | 19 ++++- src/_protowire/module.cc | 146 +++++++++++++++++++++++++++++++++---- src/protowire/pxf.py | 51 ++++++++++++- tests/test_pxf_bind_row.py | 76 +++++++++++++++++++ 4 files changed, 270 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f73c12..4c22665 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,21 @@ format changes. ### Changed +- **A row read as cells keeps its list and block values and its precision + suffixes** (#47), as protowire-go's `DatasetRow` keeps them. So a row from + `DatasetReader.next_or_none`, iteration or `Result.datasets`, passed to + `bind_row`, binds as `DatasetReader.scan` binds it. Before, a list or block + cell came back as `("unknown", None)` and `bind_row` refused it, and `1.5p80` + came back as `("float", "1.5")`, losing its precision. The public `Cell` type + changes: + - New kinds `("list", [cell, ...])` and `("block", [entry, ...])`. An entry + is a `(kind, key, value, quoted)` tuple of kind `"field"` (`k = v`), + `"map"` (`k: v`), `"block"` (`k { ... }`) or `"type"` (`@type = "url"`). + `quoted` records a key written as a string literal. + - An `"int"` or `"float"` cell's raw text keeps its precision suffix: + `("float", "1.5p80")`. **`float(cell[1])` now raises on such a cell**, + where before it silently dropped the precision. + - **`pxf.bind_row` and `DatasetReader.scan` bind a row as protowire-go does** (#24), through protowire-cpp's `BindRow` rather than writing the row out as text and decoding it with `unmarshal`. On all 43 of the spec's @@ -21,8 +36,8 @@ format changes. differently. What changes: - An absent cell takes the field's `(pxf.default)`. An absent `(pxf.required)` field is an error, as in `unmarshal_full`. - - A cell may hold a list or a block (`scan` only; see #47 for rows read as - cells), and a column may be a dotted path into a sub-message. + - A cell may hold a list or a block, and a column may be a dotted path + into a sub-message. - **A row binds into what the message already holds,** which is no longer cleared first. In a reused message, a field an earlier row set keeps its value under an empty or `null` cell, as in Go. Bind each row into a new or diff --git a/src/_protowire/module.cc b/src/_protowire/module.cc index 097fe73..53c9c1d 100644 --- a/src/_protowire/module.cc +++ b/src/_protowire/module.cc @@ -85,20 +85,56 @@ const pbuf::Descriptor* FindDescriptor(const SchemaBundle& s, // CellToPyTuple converts a single AST cell value (or std::nullopt for an // absent cell) into the FFI shape consumed by pxf.py — `None` for absent, -// `(kind, value)` otherwise. Used by PxfUnmarshalFull for @dataset rows. +// `(kind, value)` otherwise. It keeps everything protowire-go's DatasetRow +// cell keeps (#47). // // kind values mirror the AST variant tags: // "null" → nb::none() // "string" → str (already-unescaped UTF-8) -// "int" → str (raw integer text — Python wrapper decides parse) -// "float" → str (raw float text) +// "int" → str (raw integer text, its precision suffix included: +// "float" → str "1.5p80". The parser keeps the suffix apart, in +// prec; PXF numbers are decimal, so the 'p' is +// unambiguous and PyCellToValue splits it off again.) // "bool" → bool // "bytes" → bytes // "ident" → str // "timestamp" → str (raw RFC3339) // "duration" → str (raw duration) -nb::object CellToPyTuple(const std::optional& cell) { - if (!cell.has_value()) return nb::none(); +// "list" → list of cells (an element is never absent) +// "block" → list of entries, each (kind, key, value, quoted): +// ("field", name, cell, quoted) name = value +// ("map", key, cell, quoted) key: value +// ("block", name, [entry, ...], quoted) name { ... } +// ("type", url, None, False) @type = "url" +// quoted records a key written as a string literal. +nb::object ValueToPy(const protowire::pxf::ValuePtr& value); + +nb::object EntryToPy(const protowire::pxf::EntryPtr& entry) { + using namespace protowire::pxf; + return std::visit( + [](const auto& p) -> nb::object { + using T = std::decay_t; + if constexpr (std::is_same_v) { + return nb::make_tuple(std::string("field"), p->key, ValueToPy(p->value), p->key_quoted); + } else if constexpr (std::is_same_v) { + return nb::make_tuple(std::string("map"), p->key, ValueToPy(p->value), p->key_quoted); + } else if constexpr (std::is_same_v) { + nb::list entries; + for (const auto& e : p->entries) entries.append(EntryToPy(e)); + return nb::make_tuple(std::string("block"), p->name, entries, p->name_quoted); + } else { + static_assert(std::is_same_v); + return nb::make_tuple(std::string("type"), p->url, nb::none(), false); + } + }, + entry); +} + +std::string WithPrec(const std::string& raw, const std::string& prec) { + return prec.empty() ? raw : raw + "p" + prec; +} + +nb::object ValueToPy(const protowire::pxf::ValuePtr& value) { using namespace protowire::pxf; return std::visit( [](const auto& p) -> nb::object { @@ -108,9 +144,9 @@ nb::object CellToPyTuple(const std::optional& cell) { } else if constexpr (std::is_same_v) { return nb::make_tuple(std::string("string"), p->value); } else if constexpr (std::is_same_v) { - return nb::make_tuple(std::string("int"), p->raw); + return nb::make_tuple(std::string("int"), WithPrec(p->raw, p->prec)); } else if constexpr (std::is_same_v) { - return nb::make_tuple(std::string("float"), p->raw); + return nb::make_tuple(std::string("float"), WithPrec(p->raw, p->prec)); } else if constexpr (std::is_same_v) { return nb::make_tuple(std::string("bool"), p->value); } else if constexpr (std::is_same_v) { @@ -123,13 +159,26 @@ nb::object CellToPyTuple(const std::optional& cell) { return nb::make_tuple(std::string("timestamp"), p->raw); } else if constexpr (std::is_same_v) { return nb::make_tuple(std::string("duration"), p->raw); + } else if constexpr (std::is_same_v) { + nb::list elements; + for (const auto& e : p->elements) elements.append(ValueToPy(e)); + return nb::make_tuple(std::string("list"), elements); + } else if constexpr (std::is_same_v) { + nb::list entries; + for (const auto& e : p->entries) entries.append(EntryToPy(e)); + return nb::make_tuple(std::string("block"), entries); } else { - // List / Block are rejected at @dataset cell-parse time, so this - // branch is unreachable for cells. Surface as a clean error. + // BadVal: a parse error's placeholder. A reader that returned the + // row has already reported the error, so this is not reached. return nb::make_tuple(std::string("unknown"), nb::none()); } }, - *cell); + value); +} + +nb::object CellToPyTuple(const std::optional& cell) { + if (!cell.has_value()) return nb::none(); + return ValueToPy(*cell); } // The decode limits of HARDENING.md § Mandatory limits, as Python passes them: @@ -159,11 +208,61 @@ void ApplyLimits(const PxfLimits& l, protowire::pxf::UnmarshalOptions& opts) { // PyCellToValue is CellToPyTuple's inverse, for a cell pxf.py has already // normalized: None for absent, else (kind, value) with value a str, a bool -// ("bool") or bytes ("bytes"). A value cell that crossed to Python as -// ("unknown", None), a list or a block, cannot come back. -std::optional PyCellToValue(nb::handle cell) { +// ("bool"), bytes ("bytes"), a list of cells ("list") or a list of +// (kind, key, value, quoted) entries ("block"). An "int" or "float" raw +// text's precision suffix goes back to prec. +protowire::pxf::ValuePtr PyToValue(nb::handle cell); + +protowire::pxf::EntryPtr PyToEntry(nb::handle entry) { using namespace protowire::pxf; - if (cell.is_none()) return std::nullopt; + auto t = nb::cast(entry); + if (t.size() != 4) { + throw nb::value_error("pxf.bind_row: a block entry is a (kind, key, value, quoted) tuple"); + } + const auto kind = nb::cast(t[0]); + auto key = nb::cast(t[1]); + const bool quoted = nb::cast(t[3]); + if (kind == "field") { + auto p = std::make_unique(); + p->key = std::move(key); + p->key_quoted = quoted; + p->value = PyToValue(t[2]); + return EntryPtr(std::move(p)); + } + if (kind == "map") { + auto p = std::make_unique(); + p->key = std::move(key); + p->key_quoted = quoted; + p->value = PyToValue(t[2]); + return EntryPtr(std::move(p)); + } + if (kind == "block") { + auto p = std::make_unique(); + p->name = std::move(key); + p->name_quoted = quoted; + for (nb::handle e : nb::cast(t[2])) p->entries.push_back(PyToEntry(e)); + return EntryPtr(std::move(p)); + } + if (kind == "type") { + auto p = std::make_unique(); + p->url = std::move(key); + return EntryPtr(std::move(p)); + } + throw nb::value_error(("pxf.bind_row: unknown block entry kind '" + kind + "'").c_str()); +} + +// SplitPrec splits "1.5p80" into raw "1.5" and prec "80". PXF numbers are +// decimal, so a 'p' can only start the suffix. +template +void SplitPrec(const std::string& text, T& v) { + const auto at = text.find('p'); + v.raw = text.substr(0, at); + if (at != std::string::npos) v.prec = text.substr(at + 1); +} + +protowire::pxf::ValuePtr PyToValue(nb::handle cell) { + using namespace protowire::pxf; + if (cell.is_none()) throw nb::value_error("pxf.bind_row: a list element cannot be absent"); auto t = nb::cast(cell); if (t.size() != 2) throw nb::value_error("pxf.bind_row: a cell is None or a (kind, value) pair"); const auto kind = nb::cast(t[0]); @@ -176,12 +275,12 @@ std::optional PyCellToValue(nb::handle cell) { } if (kind == "int") { auto p = std::make_unique(); - p->raw = nb::cast(v); + SplitPrec(nb::cast(v), *p); return ValuePtr(std::move(p)); } if (kind == "float") { auto p = std::make_unique(); - p->raw = nb::cast(v); + SplitPrec(nb::cast(v), *p); return ValuePtr(std::move(p)); } if (kind == "bool") { @@ -211,9 +310,24 @@ std::optional PyCellToValue(nb::handle cell) { p->raw = nb::cast(v); return ValuePtr(std::move(p)); } + if (kind == "list") { + auto p = std::make_unique(); + for (nb::handle e : nb::cast(v)) p->elements.push_back(PyToValue(e)); + return ValuePtr(std::move(p)); + } + if (kind == "block") { + auto p = std::make_unique(); + for (nb::handle e : nb::cast(v)) p->entries.push_back(PyToEntry(e)); + return ValuePtr(std::move(p)); + } throw nb::value_error(("pxf.bind_row: unknown cell kind '" + kind + "'").c_str()); } +std::optional PyCellToValue(nb::handle cell) { + if (cell.is_none()) return std::nullopt; + return PyToValue(cell); +} + // NewMessageFrom makes a message of desc holding msg_bytes. A row binds into // what the message already holds, as protowire-go's BindRow and Scan do: they // do not reset it first. diff --git a/src/protowire/pxf.py b/src/protowire/pxf.py index 57dac74..cffbccb 100644 --- a/src/protowire/pxf.py +++ b/src/protowire/pxf.py @@ -28,17 +28,38 @@ # ---- ---------- ----- # "null" None present-but-null (draft §3.9) # "string" str escape-decoded UTF-8 -# "int" str raw text — Python wrapper leaves parse to caller -# "float" str raw text +# "int" str raw text — Python wrapper leaves parse to caller. +# A precision suffix is kept: "1.5p80". +# "float" str raw text, as for "int" # "bool" bool true / false # "bytes" bytes base64-decoded # "ident" str unquoted identifier (typically an enum tag name) # "timestamp" str raw RFC3339 # "duration" str raw duration text +# "list" list[Cell] `[a, b]`; an element is never None +# "block" list[Entry] `{ ... }`, its entries in order +# +# An Entry is a (kind, key, value, quoted) tuple: +# +# kind key value written as +# ---- --- ----- ---------- +# "field" field name Cell name = value +# "map" map key Cell key: value +# "block" field name list[Entry] name { ... } +# "type" type URL None @type = "url" +# +# quoted is True when the key was written as a string literal (`"k": v`), +# which tells the string map key "1" from the integer key 1. +# +# This keeps what protowire-go's DatasetRow keeps (#47), so a row read as +# cells binds with bind_row as DatasetReader.scan binds it. CellKind = Literal[ - "null", "string", "int", "float", "bool", "bytes", "ident", "timestamp", "duration" + "null", "string", "int", "float", "bool", "bytes", "ident", "timestamp", "duration", + "list", "block", ] +EntryKind = Literal["field", "map", "block", "type"] Cell = Union[None, tuple[CellKind, object]] +Entry = tuple[EntryKind, str, object, bool] @dataclass(frozen=True) @@ -621,9 +642,16 @@ def bind_row( def _native_cell(cell: Cell | None): """A cell in the shape the native bind_row takes: None, or (kind, value) - with value a str, or a bool for "bool" and bytes for "bytes".""" + with value a str, a bool for "bool", bytes for "bytes", a list of cells + for "list" and a list of entries for "block".""" if cell is None: return None + return _native_value(cell) + + +def _native_value(cell): + if cell is None: + raise ValueError("bind_row: a list element cannot be absent (None)") kind, value = cell if kind == "null": return ("null", None) @@ -633,4 +661,19 @@ def _native_cell(cell: Cell | None): return (kind, bool(value)) if kind == "bytes": return (kind, bytes(value)) + if kind == "list": + return (kind, [_native_value(e) for e in value]) + if kind == "block": + return (kind, [_native_entry(e) for e in value]) raise ValueError(f"bind_row: unknown cell kind {kind!r}") + + +def _native_entry(entry): + kind, key, value, quoted = entry + if kind in ("field", "map"): + return (kind, str(key), _native_value(value), bool(quoted)) + if kind == "block": + return (kind, str(key), [_native_entry(e) for e in value], bool(quoted)) + if kind == "type": + return (kind, str(key), None, False) + raise ValueError(f"bind_row: unknown block entry kind {kind!r}") diff --git a/tests/test_pxf_bind_row.py b/tests/test_pxf_bind_row.py index 87ac797..f15a73f 100644 --- a/tests/test_pxf_bind_row.py +++ b/tests/test_pxf_bind_row.py @@ -303,3 +303,79 @@ def test_every_spec_dataset_fixture_streams_as_go(): for f in fds.file: pool.Add(f) assert _stream_all(pool) == GOLDEN.read_text() + + +# --- a row read as cells binds as scan binds it (#47) ------------------------- +# +# Cells keep what protowire-go's DatasetRow keeps: a list's elements, a +# block's entries, a number's precision suffix. So a row read with +# next_or_none and passed to bind_row gives scan's message, which is Go's. + +_CELL_FIXTURES = ("list-cell", "block-cell", "nested-cells", "cell-framing") + + +@pytest.mark.parametrize("fixture", _CELL_FIXTURES) +def test_a_row_read_as_cells_binds_as_scan_binds_it(all_types_cls, fixture): + reader = pxf.DatasetReader.from_bytes(_FIXTURES[fixture]) + got = [] + for row in reader: + msg = all_types_cls() + pxf.bind_row(msg, reader.columns, row) + got.append(msg.SerializeToString(deterministic=True).hex()) + assert got == _ACCEPT[fixture] + + +def test_list_and_block_cells_keep_their_parts(): + row = pxf.DatasetReader.from_bytes( + '@dataset x.R (a, b)\n' + '([1, "s", [2]], { @type = "t/x" k = 1 m { n = 2 } "q": 3 r: null })\n' + ).next_or_none() + assert row == ( + ("list", [("int", "1"), ("string", "s"), ("list", [("int", "2")])]), + ("block", [ + ("type", "t/x", None, False), + ("field", "k", ("int", "1"), False), + ("block", "m", [("field", "n", ("int", "2"), False)], False), + ("map", "q", ("int", "3"), True), + ("map", "r", ("null", None), False), + ]), + ) + + +def _bigfloat_holder(): + F = descriptor_pb2.FieldDescriptorProto + bignum = descriptor_pb2.FileDescriptorProto(name="pxf/bignum.proto", package="pxf", + syntax="proto3") + bf = bignum.message_type.add(name="BigFloat") + bf.field.add(name="mantissa", number=1, type=F.TYPE_BYTES) + bf.field.add(name="exponent", number=2, type=F.TYPE_INT32) + bf.field.add(name="prec", number=3, type=F.TYPE_UINT32) + bf.field.add(name="negative", number=4, type=F.TYPE_BOOL) + fd = descriptor_pb2.FileDescriptorProto(name="bf.proto", package="bf", syntax="proto3") + fd.dependency.append("pxf/bignum.proto") + h = fd.message_type.add(name="H") + h.field.add(name="f", number=1, type=F.TYPE_MESSAGE, type_name=".pxf.BigFloat") + pool = descriptor_pool.DescriptorPool() + pool.Add(bignum) + pool.Add(fd) + return message_factory.GetMessageClass(pool.FindMessageTypeByName("bf.H")) + + +def test_a_precision_suffix_survives_the_cell(): + holder = _bigfloat_holder() + src = "@dataset bf.H (f)\n(1.5p80)\n" + reader = pxf.DatasetReader.from_bytes(src) + row = reader.next_or_none() + assert row == (("float", "1.5p80"),) + from_cells = holder() + pxf.bind_row(from_cells, reader.columns, row) + scanned = holder() + assert pxf.DatasetReader.from_bytes(src).scan(scanned) + assert from_cells.f.prec == 80 + assert from_cells.SerializeToString(deterministic=True) == \ + scanned.SerializeToString(deterministic=True) + + +def test_a_list_element_cannot_be_absent(all_types_cls): + with pytest.raises(ValueError, match="list element cannot be absent"): + pxf.bind_row(all_types_cls(), ("repeated_string",), (("list", [None]),))