Replacing Option<Vec<_>> with Vec<_>? in ExecutionPlan

[mustafasrepo]

In the codebase fn output_ordering(&self) API of ExecutionPlan returns Option<&[PhysicalSortExpr]>.

Similarly, fn required_input_ordering(&self) API returns Option<Vec<PhysicalSortRequirement>> for each children.

I think, we can replace these APIs with the version without Option (e.g &[PhysicalSortExpr], Vec<PhysicalSortRequirement> respectively). I think this would simplify code a bit ,improve readability. The information represented with None is same with empty vector.

I wonder what community thinks about this? Should we do this change, or is existing API better? (If so, how?)

Thanks in advance.

[alamb]

If we are going to change output_ordering I would like to propose a slightly different API, specifically a named struct

So the API perhaps could look something like

enum OutputOrdering {
  Unknown,
  Known(Vec<PhysicalSortExpr]>),
}


...
trait ExecutionPlan {
  fn output_ordering(&self) -> &OutputOrdering ;
...
}

The rationale to make an enum is twofold:

1. I think we could make the code even simpler with the appropriate constructor and From impls and we can encapsulate common sort order creation functions as methods
2. we could evolve its implementation in the future by changing how OutputOrdering is constructed rather than the signature.

I think making a structure would be even more helpful for required_input_ordering whose signature is actually Vec<Option<Vec<Option<...>>>>

[mustafasrepo]

I aggre with you, wrapping ordering and requirement to its own struct or enum would be better. And will make it easy to change codebase in the future. However, similar to the my original question.
What about using

struct OutputOrdering {
  inner: Vec<PhysicalSortExpr]>,
}

instead of

enum OutputOrdering {
  Unknown,
  Known(Vec<PhysicalSortExpr]>),
}

Do you think we need to distinguish empty vector, and Unknown cases?.

[alamb]

I think if the implementation is wrapped in a struct that hides the implementation details, I don't have a strong preference. It would be nice to avoid creating / copying lots of Vec's around.