diff --git a/CHANGELOG.md b/CHANGELOG.md index 725a003..7051b8d 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 dac2a79..f381468 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) @@ -617,9 +638,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) @@ -629,4 +657,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]),))