feat(core): handle RelCommon data for every relation type

[nielspardon]

Hint was wired into most but not all relations, and each of the ~31 newXxx methods in ProtoRelConverter repeated the same four RelCommon setters by hand. Two had drifted: Cross dropped its hint and TopN dropped its rel anchor when converting from proto.

The proto → POJO direction is now centralised in ProtoRelConverter.applyRelCommon, mirroring RelProtoConverter.common on the way out, so the emit mapping, common extension, hint and rel anchor are applied in one place. This needs type-agnostic withRemap / withCommonExtension / withHint copy methods on Rel alongside the existing withRelAnchor; they are useful for plan rewriting in their own right. applyRelCommon copies a field only when it differs, so it stays a no-op for a common { direct {} } message — including for custom, non-Immutables relations, which inherit Rel's throwing defaults.

Two data-loss bugs surfaced along the way:

• RelProtoConverter.common never wrote RelCommon.Hint.advanced_extension, so Hint.getExtension() was silently dropped on every relation even though the inbound direction read it back.
• UpdateRel has no common field (spec v0.99.0), but the RelCommon accessors live on Rel, so a NamedUpdate can hold them — and an emit mapping changes getRecordType(). Serialising one produced a plan whose schema disagreed with the POJO. visit(NamedUpdate) now rejects it instead of dropping it silently. This is a behaviour change: it makes SubstraitBuilder.namedUpdate(..., Rel.Remap) fail at serialisation rather than emit a plan that cannot be re-imported. The restriction is temporary — fix: add missing RelCommon field to UpdateRel substrait#1168 adds common to UpdateRel, and once that ships the guard and its carve-out in RelCommonRoundtripTest come back out.

RelCommonRoundtripTest covers one sample of every RelVisitor relation type and fails when a new relation has no sample. It is keyed on POJO types rather than proto oneof cases, so a new sub-case mapping onto an existing POJO type still needs care — noted in AGENTS.md.

Closes #297

🤖 Generated with AI

[alexandrefimov]

SubstraitBuilder.namedUpdate(…, Rel.Remap) exists solely to put a remap on a NamedUpdate, so from here on every plan built through it throws at serialisation. Worth dropping or deprecating that overload in this PR — otherwise the DSL keeps handing out a relation whose only possible outcome is this exception.

[nielspardon]

Deliberately not deprecating: substrait-io/substrait#1168 adds RelCommon common = 6 to UpdateRel, so this overload becomes valid again once that lands and we bump substrait-packaging. Deprecating now would mean un-deprecating a release later.

Documented the limitation on the overload instead — it now says the returned relation can't be serialized while UpdateRel has no common field, and points at the 5-arg overload for anything that has to round-trip.

[alexandrefimov]

That makes sense. The Javadoc now makes the current limitation clear, and avoiding a temporary deprecation is reasonable given substrait-io/substrait#1168. Thanks.

[alexandrefimov]

ReferenceRel has no common field either — javap over protobuf 0.99.0 gives UpdateRel, ReferenceRel and the PlanRel/Rel wrappers. No POJO models it today, so nothing is broken, but "the only such message" (here and on checkNoRelCommon) is what whoever models ReferenceRel will read. "the only modelled one" stays true either way.

[nielspardon]

Right, fixed — reproduced the same three (PlanRel, ReferenceRel, UpdateRel) with javap. Now "the only modeled message without one" here, on checkNoRelCommon, in the newNamedUpdate comment and in AGENTS.md, each noting ReferenceRel has none either but isn't modeled.

On checkNoRelCommon I also carried over why substrait-io/substrait#1168 leaves ReferenceRel alone: it points at another subtree rather than producing output of its own, so the common fields don't mean the same thing there. That's the bit worth having if someone does model it.

[alexandrefimov]

Thanks, the wording is accurate now, and the ReferenceRel distinction gives useful context.