fix(isthmus)!: apply the emit mapping a virtual table carries - #1189
Conversation
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
One first: the mapping is applied against the wrong basis when the scan also carries a projection. The rest are smaller -- a type lookup by name, the hint diverging between the two encodings, and the two refusal messages. #1186 already tracks the CTAS input three lines below the new write guard being converted twice.
…he ones that cannot be A VirtualTableScan routed neither its emit mapping nor its hint, so a scan whose mapping drops a column converted carrying all of them, with a row type that reported one. It goes through applyRelCommon now, like every other relation whose conversion applies one. A NamedWrite and a NamedDdl cannot: a TableModify's row type is a single ROWCOUNT column, and a CreateTable or a CreateView produces the object it creates. They refuse a mapping the way visit(NamedUpdate) already does, rather than dropping it. Closes substrait-io#1160
Nothing covered the case a reader asks about first: with no mapping there is no projection for the names to land on, so the table keeps the names its schema gives it.
6ee57a6 to
30f8219
Compare
|
One thing to settle before either of these lands: #1151 replaces this method with an emitter for an isthmus-owned |
|
This one first, agreed — #1151 can rebase onto it with the projection guard and the by-index selection already settled. Keeping them as two reviews. |
2 participants
visit(VirtualTableScan)routed neither half of the relation'sRelCommon: a scan whose emit mapping drops a column converted carrying all of them, under a row type that reported one, and the names of its hint were dropped with it. It goes throughapplyRelCommonnow, like every relation whose conversion applies a mapping.A
NamedWriteand aNamedDdlcannot apply one, for reasons that are isthmus' rather than the relation's. A write's record type is its input's, but isthmus converts it to aTableModifywhose row type is a single ROWCOUNT column, dropping the write'soutput_modealong the way, so the mapping has nothing left to select from; and there is no place for a projection between aCreateView's definition and the view it creates.visit(NamedUpdate)already refuses one, and these two now do the same instead of dropping it.A scan carrying a
projectionis refused as well.AbstractReadRel.deriveRecordType()masks the initial schema with it before anything else, so an emit mapping's indices count the columns the mask leaves -- while isthmus builds the row type from the unmasked schema and reads the projection nowhere.ProtoRelConverterfills the field from the proto, so such a relation reaches the conversion.The mapping selects its columns by index rather than by name. A virtual table's row type carries the names of its schema, which Calcite never uniquifies, so resolving a field by name gave the projection the type of the first column sharing it: an
AssertionErrorunder-ea, a silently mistyped plan without one.A mapping is materialised as a projection, so a scan carrying one comes back from Calcite as a projection over a virtual table rather than as the scan it went in as -- the shape every relation with a mapping returns as. Without a mapping there is no projection for the hint's names to land on, and the table keeps the names its schema gives it.
Closes #1160
BREAKING CHANGE: a NamedWrite or a NamedDdl carrying an emit mapping, and a VirtualTableScan carrying a projection, no longer convert to Calcite. They used to convert with the mapping or the projection dropped, which produced a plan that did not describe the relation it came from.