On Mon, Aug 19, 2019 at 9:16 AM Ben Kietzman <ben.kietz...@rstudio.com> wrote:
>
> Thanks for responding.
>
> I can certainly add VisitBuilder/VisitBuilderInline for ArrayBuilder, but
> there's a slight difficulty: some types don't have a single concrete
> builder class. For example, the builders of dictionary arrays are templated
> on the encoded type and also on an index builder, which is either adaptive
> or fixed at int32. I can make this transparent in the definition of
> VisitBuilder so that one can add Visit(StringDictionaryBuilder*) and
> Visit(DictionaryBuilder<Int64Type>*). Something similar is already required
> for IntervalType, which may be MonthIntervalType or DayTimeIntervalType
> depending on the value of IntervalType::interval_type().
>
> Another difficulty is the adaptive builders. Whatever they return for
> ArrayBuilder::type()/type_id()/..., it won't help to discriminate them from
> the non adaptive builders. Is there any application which uses the adaptive
> builders except the dictionary builders? If possible, I think it would be
> simplest to make their inheritance of ArrayBuilder protected.
>
My gut feeling is that ArrayBuilder::type virtual is the simplest
thing and causes the complexity relating to changing types be
localized to classes like AdaptiveIntBuilder. I don't have a sense for
what use cases changing from an inline member (type_) to a virtual
function would cause a meaningful performance issue. You could even
play tricks by having something like
sp<DataType> type() const {
if (!is_fixed_type_) {
// type_ is null, GetCurrentType is a protected virtual
return GetCurrentType();
} else {
return type_;
}
}
You said "much more expensive for nested builders" -- where and how exactly?
> Ben
