fix(isthmus)!: round-trip a virtual table whose rows are not all literals - #1151
fix(isthmus)!: round-trip a virtual table whose rows are not all literals#1151alexandrefimov wants to merge 6 commits into
Conversation
|
Thanks for digging into this. I'd like to propose a different approach before you put more into this one, because I think the shape matcher will keep costing us. What I'd like to try instead: an isthmus-owned Calcite relation carrying the virtual table's rows as Then, for consumers who need stock Calcite, a The reason I'd rather not refine the matcher: it accepts a strictly larger set of unions than the emitter produces, and that gap is doing real damage — arms differing only in nullability or width now throw |
|
All three check out; measurements below so nobody has to redo them. The test is vacuous as you say. Reverting both main files leaves The nullability gap is a live regression rather than a corner. A union of two single-row projections differing only in nullability: The matcher takes the schema from And it does not survive planning. A single-row virtual table emits a one-input union:
So I would rather rework it your way than refine the matcher: recognition by identity cannot over-match, a rule cannot erase it, and carrying the schema on the node is the part that reaches the nested-struct case. I will take that on. One thing I would like your read on before I start. #1153 changes |
I did post review comments on #1153 just now. We can tackle that on its own. |
70d4672 to
be75944
Compare
|
Reworked the way you proposed and pushed. The branch sits on top of #1153, so the first two commits here are that one. Two things the description does not carry. One cost the shape match never had: a relation holding The other is a gain: the struct-column round trip comes back here. #1153 has to give it up, since a row literal cannot sit in a Values tuple and the column takes the projection encoding there, while on this branch |
3cf8fec to
4a66745
Compare
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Run VirtualTableExpansionRule inside SubstraitToSql and SubstraitSqlDialect.toSql before RelToSqlConverter — both now throw on any plan with a computed virtual table, and that's the call I'd like your read on, since isthmus's own SQL renderer turns out to be the consumer the rule exists for.
Two more blockers inline: a computed field inside a nullable struct crashes converting back, and accept(RexShuttle) keeps a stale rowType.
Please also rebase — main has moved five commits since your base — and drop the "first two commits here are #1153" line from the description, since it's merged and there are two commits of your own now.
…rals Calcite has no relation that carries expressions the way a Substrait virtual table does, so the conversion expanded one into a UNION ALL of a single-row projection per row. Nothing in that shape says it was a table: converting the plan back gave the projection, and the relation changed under a round trip. Emit an isthmus-owned relation instead -- VirtualTable, holding the rows as RexNodes and the schema as its row type -- and recognise it by type on the way back, the way CreateTable and CreateView already are. Recognition by type cannot over-match a union someone wrote out of the same parts, and no planner rule can erase it: UNION_REMOVE strips the one-input union a single-row table expanded to, and UNION_MERGE rewrites into the same shape. Carrying the schema on the relation is also what reaches a struct column, whose names cannot be paired with a row type after the fact. A consumer whose planner only knows Calcite's own relations can expand the table with VirtualTableExpansionRule. That is opt-in, isthmus never runs it, and it is one-way: the expansion converts back as the projection it is. BREAKING CHANGE: a virtual table whose rows are not all literals now converts to io.substrait.isthmus.calcite.rel.VirtualTable instead of a UNION ALL of single-row projections over an empty table. Add VirtualTableExpansionRule to a planner to get that shape back.
copy() rejecting an input and the expansion of a table with no rows were both unpinned: deleting either left the suite green. The DDL relations next door have the same input check and cover it.
… relation carry its own rules
4a66745 to
6144a7a
Compare
…ns declare A literal is converted at its column's declared type, the way visit(Values) does it, so a value Calcite inferred a narrower type for does not disagree with the schema built from the same row type. A value that is not a literal cannot be given that type: it is converted from the expressions it is built of and takes its type from them, and casting at the declared type would put an expression in the output the input did not have. That shape is refused here, naming the value and both types, rather than left to VirtualTableScan, whose check compares the two without promoting either. A computed field inside a nullable struct is where the two meet: Calcite pushes the struct's nullability into its fields, so the schema taken from the row type has nullable fields while the expression keeps its own.
A subquery's relation is not an input, so walking inputs never reaches a correlation it declares; the row walk added here is what does. The test puts a subquery in a row of a virtual table that is the right input of a correlate: the enclosing relation binds its own id, and the filter inside the row's subquery binds a second one that only the row walk reaches. Removing the walk fails it, and nothing else. The table declares no correlation variables of its own -- an id binds to a relation whose fields the reference names, and a leaf has none -- which its accessor now says.
The generated implementation carries javax.annotation.Nullable for the nullable description RelRule.Config declares, in the builder method that copies from the supertype. Value.Style's allowedClasspathAnnotations, nullableAnnotation and fallbackNullableAnnotation do not reach it there, which is worth saying where the hand-written configuration is.
2 participants
Calcite has no relation that carries expressions the way a Substrait virtual table does, so a
VirtualTableScanwhose rows contain a call was expanded into aUNION ALLof one single-row projection per row, each over an emptyValues. Nothing in that shape says it was a table, so the plan came back as aSetofProjects over empty virtual tables -- a different relation than the one that went in. The names went with it: the schema's names rode on a renaming projection above the union, and that projection is merged into whatever projection sits above the table.Emit an isthmus-owned relation instead.
VirtualTableholds the rows asRexNodes and the schema as its row type, andvisitOtherrecognises it by type, the wayCreateTableandCreateViewalready are.Recognition by type cannot over-match: a union someone wrote out of the same parts stays a union, and so does one whose arms differ only in nullability -- a shape match takes the rows from the arms and the schema from the union's row type, so anything the least-restrictive type widened is rejected against the schema. And no planner rule can erase it:
UNION_REMOVEstrips the one-input union a single-row table expands to, andUNION_MERGEflattens a nested union of single-row projections into exactly that shape (both measured).Carrying the schema on the relation is also what reaches a struct column: a schema names every field at every level, depth-first, which cannot be paired with a row type after the fact.
For a consumer whose planner only knows Calcite's own relations,
VirtualTableExpansionRuleexpands the table into the union of projections. Isthmus runs it in one place, its own SQL generation:RelToSqlConverter.visit(RelNode)throws anAssertionErrornaming any relation it does not know, unconditionally, so bothSubstraitToSql.convertandSubstraitSqlDialect.toSqlexpand before handing it the tree. The expansion is one-way everywhere else: it is a plan like any other and converts back as the relation it is.A row's values are converted at the types their columns declare, the way
visit(Values)does it, so a literal Calcite inferred a narrower type for still matches the schema built from the same row type. A value that is not a literal cannot be given that type, since it is converted from the expressions it is built of, and casting at the declared type would put an expression in the output the input did not have — so where the two differ the conversion is refused, naming the value and both types. A computed field inside a nullable struct is the shape that reaches it: Calcite pushes the struct's nullability down into its fields while the expression keeps its own.Closes #631
BREAKING CHANGE: a virtual table whose rows are not all literals now converts to
io.substrait.isthmus.calcite.rel.VirtualTableinstead of aUNION ALLof single-row projections over an empty table. A consumer handing the tree to something that knows Calcite's own relations only needsVirtualTableExpansionRuleto get that shape back; isthmus' own SQL generation runs it already.