Minor: [C++] Fix sink schema #12715
Minor: [C++] Fix sink schema #12715
Conversation
|
IMO, this isn't minor anymore - we should file a Jira. |
|
@lidavidm I will create one. |
|
The approach looks fine to me, but I thought there was some reason why we didn't want a SinkNode to technically have an output schema. @westonpace do you recall? |
| {"order_by_sink", OrderBySinkNodeOptions{options, &sink_gen}}, | ||
| }) | ||
| .AddToPlan(plan.get())); | ||
| ASSERT_EQ(plan->sources()[0]->output_schema(), plan->sinks()[0]->output_schema()); |
There was a problem hiding this comment.
Is it possible to just add these checks to existing tests?
There was a problem hiding this comment.
Yes we can. I separated it in case it diverts from the purpose. There is code duplication with the current approach.
|
Hmm, I don't, but thinking about it now I guess I can kind of see why it is this way. Technically the sink node has no outputs so there is no input schema. I suppose what we really want is for the sink consumer to access the schema of the input node. Take a look at #12721 and see if that might be a better approach. |
|
@westonpace @lidavidm let's close this PR. I am +1 for #12721 |
|
Closing this PR inplace of #12721 |
3 participants
This PR includes a minor suggestion (suggestive-PR) on fixing the issue associated with obtaining the
output_schemafrom the streaming executionplan.