FORM readers serve the wrong event: 1-based row bookkeeping vs 0-based container reads

[olantwin]

StorageReader hands out 1-based row numbers:

• the sequential fast path maps [event:N] to N + 1 (sequential_row_from_index_id,

  phlex/form/storage/storage_reader.cpp

  Lines 159 to 186 in 88ed89b

  	std::optional<int> sequential_row_from_index_id(std::string const& id)
  	{
  	if (id == "[]") {
  	return 1;
  	}
  	if (id.size() < 2 || id.front() != '[' || id.back() != ']') {
  	return std::nullopt;
  	}
  	auto const body = id.substr(1, id.size() - 2);
  	auto const comma = body.find(',');
  	if (comma != std::string::npos) {
  	return std::nullopt;
  	}
  	auto const colon = body.find(':');
  	if (colon == std::string::npos) {
  	return std::nullopt;
  	}
  	try {
  	auto const number = std::stoi(body.substr(colon + 1));
  	return number + 1;
  	} catch (...) {
  	return std::nullopt;
  	}
  	}

  ), and
• the index-map builder in StorageReader::getIndex starts scanning at entry = 1.

But both storage containers interpret the row as a 0-based entry number:

• RNTuple: (*m_view)(id) with bounds check id >= GetNEntries() (

  phlex/form/root_storage/root_rfield_read_container.cpp

  Lines 104 to 113 in 88ed89b

  	if (id >= (int)m_reader->GetNEntries())
  	return false;
  	//Using RNTupleView<> to read instead of reusing REntry gives us full schema evolution support: the ROOT feature that lets us read files with an old class version into a new class version's memory.
  	auto buffer = m_view->GetField().CreateObject<void>(); //PHLEX gets ownership of this memory
  	assert(buffer);
  	m_view->BindRawPtr(buffer.get());
  	try {
  	(*m_view)(id);

  )
• TTree: m_tree->LoadTree(id) with bounds check id > GetEntries() (

  phlex/form/root_storage/root_tbranch_read_container.cpp

  Lines 100 to 182 in 88ed89b

  	if (id > m_tree->GetEntries())
  	return false;
  	gsl::owner<void*> branchBuffer = nullptr;
  	auto dictInfo = TDictionary::GetDictionary(type);
  	int branchStatus = 0;
  	if (!dictInfo) {
  	throw std::runtime_error(std::string{"ROOT_TBranch_ContainerImp::read unsupported type: "} +
  	DemangleName(type));
  	}
  	if (dictInfo->Property() & EProperty::kIsFundamental) {
  	// NOLINTNEXTLINE(cppcoreguidelines-pro-type-static-cast-downcast)
  	auto fundInfo = static_cast<TDataType*>(dictInfo); // Already checked to be fundamental
  	switch (fundInfo->GetType()) {
  	case kChar_t:
  	branchBuffer = new Char_t;
  	break;
  	case kUChar_t:
  	branchBuffer = new UChar_t;
  	break;
  	case kShort_t:
  	branchBuffer = new Short_t;
  	break;
  	case kUShort_t:
  	branchBuffer = new UShort_t;
  	break;
  	case kInt_t:
  	branchBuffer = new Int_t;
  	break;
  	case kUInt_t:
  	branchBuffer = new UInt_t;
  	break;
  	case kLong_t:
  	branchBuffer = new Long_t;
  	break;
  	case kULong_t:
  	branchBuffer = new ULong_t;
  	break;
  	case kLong64_t:
  	branchBuffer = new Long64_t;
  	break;
  	case kULong64_t:
  	branchBuffer = new ULong64_t;
  	break;
  	case kFloat_t:
  	branchBuffer = new Float_t;
  	break;
  	case kDouble_t:
  	branchBuffer = new Double_t;
  	break;
  	case kBool_t:
  	branchBuffer = new Bool_t;
  	break;
  	default:
  	throw std::runtime_error(
  	std::string{"ROOT_TBranch_ContainerImp::read unsupported fundamental type: "} +
  	DemangleName(type));
  	};
  	branchStatus = m_tree->SetBranchAddress(
  	col_name().c_str(), branchBuffer, nullptr, EDataType(fundInfo->GetType()), false);
  	} else {
  	auto klass = TClass::GetClass(type);
  	if (!klass) {
  	throw std::runtime_error(std::string{"ROOT_TBranch_ContainerImp::read missing TClass"} +
  	" (col_name='" + col_name() + "', type='" + DemangleName(type) +
  	"')");
  	}
  	branchBuffer = gsl::owner<void*>(klass->New());
  	branchStatus = m_tree->SetBranchAddress(
  	col_name().c_str(), reinterpret_cast<void*>(&branchBuffer), klass, EDataType::kOther_t, true);
  	}
  	if (branchStatus < 0) {
  	throw std::runtime_error(
  	std::string{"ROOT_TBranch_ContainerImp::read SetBranchAddress() failed"} + " (col_name='" +
  	col_name() + "', type='" + DemangleName(type) + "')" + " with error code " +
  	std::to_string(branchStatus));
  	}
  	Long64_t tentry = m_tree->LoadTree(id);
  	m_branch->GetEntry(tentry);

  )

Observed with form_source on an RNTuple file (phlex v0.3.0 with the backend enabled): reading [event:0] returns event 1's data, and reading the last event fails with

FORM Error: Failed to retrieve product [particles] for [event:7]

because row 8 of an 8-entry RNTuple is out of range. The TTree backend has the mirrored problem (LoadTree(GetEntries()) returns -1, and the subsequent GetEntry(-1) leaves the buffer unfilled).

The index-map path is self-consistently shifted (it also skips the first entry when building the map), so round-trip tests that only compare counts don't catch it — the served payloads are off by one event.

I fixed it locally by making the RNTuple container honour the 1-based convention (id < 1 || id > GetNEntries() and (*m_view)(id - 1)); alternatively StorageReader could go 0-based throughout, which is probably the less surprising convention. I've added a patch in the conda-forge phlex-feedstock (conda-forge/phlex-feedstock#8), happy to upstream it or help work-out a consistent 1-based solution (which IMO should be well advertised, as I found it very surprising!)

[coderabbitai]

🔗 Related PRs

#635 - FORM RNTuple Draft [closed]
#647 - Form write output metadata [closed]
#674 - FORM read files without metadata [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[olantwin]

I believe that #715 fixes this, though I haven't had time to test yet.

[aolivier23]

That's the plan. I verified that I can read up to the last entry after #715 . I haven't actually checked that I'm reading each entry exactly once. Thank you for giving RNTuple a test drive!