Cleanup logical type construction

[benjaminwinger]

This reduces the amount of boiler-plate code that has to be written when creating logical types by moving that into types.cpp and providing more specific static methods to create the complex types, particularly since this is part of our public API and users don't really need to handle ExtraTypeInfo directly.

E.g. LogicalType::VAR_LIST(childType); instead of LogicalType(LogicalTypeID::VAR_LIST, std::make_unique<VarListTypeInfo>(childType));.

I'd also cleaned up a couple places where LogicalType was being used where LogicalTypeID could have been used instead, and removed some indirection from StructField to simplify it.

Ideally I think the ExtraTypeInfo implementations could be hidden inside types.cpp, however there are some more complex places, particularly for NODE, REL and RECURSIVE_REL, which I'd left alone.

One part I'm not sure about is the return types of the LogicalType static methods. The simple types all return unique_ptr (and the regular constructor is used in places where a unique_ptr is not wanted), but the complex types also need both, and it's a choice between:

1. Provide a separate method for each. E.g. LogicalType::VAR_LIST_P to produce a unique_ptr (or a separate method not to, for consistency with the simple types?).
2. Provide just a method which produces a LogicalType (what's implemented here), and if you want a unique_ptr wrap it with make_unique<LogicalType>(...) to move the object into the unique_ptr using the move constructor.
3. Provide just a method which produces a unique_ptr<LogicalType> and use std::move(*LogicalType::VAR_LIST(...)) to move the data out of the unique_ptr if you want (unlike (2) this adds an unnecessary heap allocation).

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ac33484) 93.15% compared to head (3799d81) 93.16%.

Files	Patch %	Lines
...rator/persistent/reader/parquet/parquet_reader.cpp	87.50%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2571   +/-   ##
=======================================
  Coverage   93.15%   93.16%           
=======================================
  Files        1027     1027           
  Lines       38414    38363   -51     
=======================================
- Hits        35785    35740   -45     
+ Misses       2629     2623    -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[andyfengHKU]

I think this is fine in this PR. But in the long run, all functions (scalar, aggregation, table, ...) should have the same base class Function whose parameters are LogicalType rather than LogicalTypeID which means we will be using LogicalType almost everywhere for function related code path.

[benjaminwinger]

Mostly it seems weird to me to pass around versions of LogicalType which have no extraTypeInfo, even for complex types, particularly when LogicalTypeID is functionally identical.

[andyfengHKU]

I thought make_unique is preferred compared to std::unique_ptr<>(new object). Is there a reason that you are changing?

[benjaminwinger]

I'd tried making the constructors private, but only the two-argument one still is. I can revert the change for the simple types, but make_unique doesn't work with private constructors.

[andyfengHKU]

One part I'm not sure about is the return types of the LogicalType static methods. The simple types all return unique_ptr (and the regular constructor is used in places where a unique_ptr is not wanted), but the complex types also need both, and it's a choice between:

I think we should just use unique_ptr everywhere or never use it. The former solution seems to be easier since we are already doing so in most places.

[acquamarin]

This helper looks wired, should we provide this helper to the user? I will let @mewim comment on this.

[benjaminwinger]

It's just to give the C API (and there's a similar one for the java API) access to the private LogicalType constructor (using a wrapper class to make it easier to declare as friend in types.h).
Maybe we should set up per-type functions similar to the LogicalType ones for constructing types (or at least, for constructing the complex types).

[acquamarin]

Why do we remove the unique_ptr here?
We always use vector<unique_ptr> to avoid unintentional copy.

[benjaminwinger]

There still is no possibility of an unintentional copy. StructField has no copy constructor (deleted implicitly since it contains a unique_ptr: E.g.). Given that its purpose is just for creating struct types, I don't think it really needs one either, so we can just leave it as a move-only type.

[benjaminwinger]

I think we should just use unique_ptr everywhere or never use it. The former solution seems to be easier since we are already doing so in most places.

I've updated it to have them return std::unique_ptr, and also made some of the places which previously took a LogicalType without the unique_ptr use a unique_ptr to compensate.