Add none type

[vasil-pashov]

Reference Issues/PRs

Resolves #639
Relates to: #540 #224

What does this implement/fix? Explain your changes.

Change the C++ to support a special undefined type that can be changed into any other type in a subsequent data key.

Any other comments?

Only the C++ layers is changed, it might need additional python layer integration.

[mehertz]

Don't leave blank please!

What is the NoneType?

[jjerphan]

Thank you for working on it, @vasil-pashov.

Here is a first review with a few questions and suggestions.

PR description added

[vasil-pashov]

This is a list of functions which assume that if something is not string it's number of vice versa:

VariantData binary_membership(const ColumnWithStrings& column_with_strings, ValueSet& value_set, Func&& func)
VariantData binary_comparator(const Value& val, const ColumnWithStrings& column_with_strings, Func&& func)
VariantData binary_comparator(const ColumnWithStrings& left, const ColumnWithStrings& right, Func&& func)
VariantData binary_comparator(const ColumnWithStrings& column_with_strings, const Value& val, Func&& func)
SegmentInMemoryImpl::visit_field(Callable &&c)
SegmentInMemoryImpl::operator==(const SegmentInMemoryImpl& left, const SegmentInMemoryImpl& right)
FrameSliceMap::FrameSliceMap(const std::shared_ptr<PipelineContext>& context, bool dynamic_schema)
std::shared_ptr<SegmentInMemoryImpl> filter(const util::BitSet& filter_bitset,
                                                       bool filter_down_stringpool=false,
                                                       bool validate=false) const {
Value(const Value& other)
void SumAggregatorData::aggregate(const std::optional<ColumnWithStrings>& input_column, const std::vector<size_t>& groups, size_t unique_values)
void MinMaxAggregatorData::aggregate(const ColumnWithStrings& input_column)
void MaxOrMinAggregatorData::aggregate(const std::optional<ColumnWithStrings>& input_column, const std::vector<size_t>& groups, size_t unique_values)
VariantData binary_membership(const ColumnWithStrings& column_with_strings, ValueSet& value_set, Func&& func) {
VariantData binary_comparator(const Value& val, const ColumnWithStrings& column_with_strings, Func&& func) {
VariantData binary_comparator(const ColumnWithStrings& left, const ColumnWithStrings& right, Func&& func) {
VariantData binary_comparator(const ColumnWithStrings& column_with_strings, const Value& val, Func&& func) {
inline StreamId stream_id_from_segment(const SegmentInMemory &seg, ssize_t row) {
folly::Future<arcticdb::entity::VariantKey> write_frame_data(const TestDataFrame &data_frame, WriterType &writer) {

they all have contain if/else case similar to:

if(is_sequence_type(dt)) {
...
} else {
...
}

or

if(is_numeric_type(dt)) {
...
} else {
...
}

Probably for some of them it's fine to leave it as is, but we might have problems down the road when none type and other types are added.

[jjerphan]

Thank you, @vasil-pashov.

Here are a few comments and questions.

William and you mentioned other PRs in the threads of reviews. Do you intend to submit subsequent PRs for the support of other types? 🙂

[jjerphan]

Should we add a else clause and raise if it's reached?

[vasil-pashov]

This is kind of doing the same thing isn't it? We should do nothing in case of empty type is passed and if dt is not empty then we have unhandled type and the code should not compile at all. This is a shorthand for:

...
else if constexpr (is_empty_type(dt)) {
} else {
            static_assert(!sizeof(dt), "Unknown data type");
}

and it's better than throwing since if we have type which is not handled we won't even compile.

[alexowens90]

This is a list of functions which assume that if something is not string it's number of vice versa:

VariantData binary_membership(const ColumnWithStrings& column_with_strings, ValueSet& value_set, Func&& func)
VariantData binary_comparator(const Value& val, const ColumnWithStrings& column_with_strings, Func&& func)
VariantData binary_comparator(const ColumnWithStrings& left, const ColumnWithStrings& right, Func&& func)
VariantData binary_comparator(const ColumnWithStrings& column_with_strings, const Value& val, Func&& func)
SegmentInMemoryImpl::visit_field(Callable &&c)
SegmentInMemoryImpl::operator==(const SegmentInMemoryImpl& left, const SegmentInMemoryImpl& right)
FrameSliceMap::FrameSliceMap(const std::shared_ptr<PipelineContext>& context, bool dynamic_schema)
std::shared_ptr<SegmentInMemoryImpl> filter(const util::BitSet& filter_bitset,
                                                       bool filter_down_stringpool=false,
                                                       bool validate=false) const {
Value(const Value& other)
void SumAggregatorData::aggregate(const std::optional<ColumnWithStrings>& input_column, const std::vector<size_t>& groups, size_t unique_values)
void MinMaxAggregatorData::aggregate(const ColumnWithStrings& input_column)
void MaxOrMinAggregatorData::aggregate(const std::optional<ColumnWithStrings>& input_column, const std::vector<size_t>& groups, size_t unique_values)
VariantData binary_membership(const ColumnWithStrings& column_with_strings, ValueSet& value_set, Func&& func) {
VariantData binary_comparator(const Value& val, const ColumnWithStrings& column_with_strings, Func&& func) {
VariantData binary_comparator(const ColumnWithStrings& left, const ColumnWithStrings& right, Func&& func) {
VariantData binary_comparator(const ColumnWithStrings& column_with_strings, const Value& val, Func&& func) {
inline StreamId stream_id_from_segment(const SegmentInMemory &seg, ssize_t row) {
folly::Future<arcticdb::entity::VariantKey> write_frame_data(const TestDataFrame &data_frame, WriterType &writer) {

they all have contain if/else case similar to:

if(is_sequence_type(dt)) {
...
} else {
...
}

or

if(is_numeric_type(dt)) {
...
} else {
...
}

Probably for some of them it's fine to leave it as is, but we might have problems down the road when none type and other types are added.

Can you add 2 tickets to deal with these. One for everything QueryBuilder related:

VariantData binary_membership(const ColumnWithStrings& column_with_strings, ValueSet& value_set, Func&& func)
VariantData binary_comparator(const Value& val, const ColumnWithStrings& column_with_strings, Func&& func)
VariantData binary_comparator(const ColumnWithStrings& left, const ColumnWithStrings& right, Func&& func)
VariantData binary_comparator(const ColumnWithStrings& column_with_strings, const Value& val, Func&& func)
std::shared_ptr<SegmentInMemoryImpl> filter(const util::BitSet& filter_bitset,
                                                       bool filter_down_stringpool=false,
                                                       bool validate=false) const {
void SumAggregatorData::aggregate(const std::optional<ColumnWithStrings>& input_column, const std::vector<size_t>& groups, size_t unique_values)
void MinMaxAggregatorData::aggregate(const ColumnWithStrings& input_column)
void MaxOrMinAggregatorData::aggregate(const std::optional<ColumnWithStrings>& input_column, const std::vector<size_t>& groups, size_t unique_values)
VariantData binary_membership(const ColumnWithStrings& column_with_strings, ValueSet& value_set, Func&& func) {
VariantData binary_comparator(const Value& val, const ColumnWithStrings& column_with_strings, Func&& func) {
VariantData binary_comparator(const ColumnWithStrings& left, const ColumnWithStrings& right, Func&& func) {
VariantData binary_comparator(const ColumnWithStrings& column_with_strings, const Value& val, Func&& func) {

and one for everything else. I can knock all the query builder ones off pretty quickly, I don't know off the top of my head about the others.