If it becomes much more expensive then calling it `type()` (rather than
e.g. GetCurrentType()) is a bit misleading.
Regards
Antoine.
Le 19/08/2019 à 16:27, Wes McKinney a écrit :
> 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
