fix: project rel to and from substrait to include pass through columns #135
Conversation
scgkiran
force-pushed
the
project_rel_take2
branch
from
7ed491d to
eebaf3d
Compare
| common = &sop.project().common(); | ||
| break; | ||
| default: | ||
| throw InternalException("Unsupported relation type " + to_string(sop.rel_type_case())); |
There was a problem hiding this comment.
Almost every relation should be in this list. I'd consider calling anything missing not yet implemented.
There was a problem hiding this comment.
Ah..! Thanks for pointing out, I missed this.
This also reminds me that I could avoid a project by using output mapping, whenever I only have to change the column order of a give relation.
| default: | ||
| throw InternalException("Unsupported relation type " + to_string(sop.rel_type_case())); | ||
| } | ||
| if (!common->has_emit()) { |
There was a problem hiding this comment.
It may be useful to break this into code to get the common table and code to get the emit from the mapping. That way you can use .the common structure to update direct and emit if necessary. You might be able to use templates to get the RelCommon which could reduce the overall amount of code.
| auto mapping = GetOutputMapping(sop); | ||
| auto num_input_columns = input_rel->Columns().size(); | ||
| if (mapping.empty()) { | ||
| for (int i = 1; i <= num_input_columns; i++) { |
There was a problem hiding this comment.
The first column is numbered at zero. I found https://substrait.io/tutorial/sql_to_substrait/#field-indices to be useful (in addition to the individual relation pages).
There was a problem hiding this comment.
The index i is used to create the duckdb column reference not substrait. For reference please see here.
No description provided.