[C++] Add Extend and ExtendMasked to the converter interface #8886
Conversation
| } | ||
|
|
||
| virtual Status Append(InputType value) = 0; | ||
| virtual Status Append(InputType value) { return Status::NotImplemented("Append"); } |
There was a problem hiding this comment.
@bkietz I think we should keep both Append and Extend since we wrap the converter object with the Chunker, so if one implementation (like the python one) choose to use Append then we don't need to subclass the chunker class.
|
Thanks for opening a pull request! Could you open an issue for this pull request on JIRA? Then could you also rename pull request title in the following format? See also: |
|
|
||
| // we could get bit smarter here since the whole batch of appendable values | ||
| // will be rejected if a capacity error is raised | ||
| Status Extend(InputType values, int64_t size) { |
There was a problem hiding this comment.
The chunker's extend wrapper is untested since we use Append from the python code. Theoretically we can wrap the converter's Extend method just like in the case of Append though we reject the whole batch rather than a single item.
We could improve this logic but would require details about the iteration.
There was a problem hiding this comment.
I think adding a int64_t start in addition to size as in #8650 would help. but I haven't yet reviewed how the chunker code works, e.g. can it "know" that it can handle n extra values in the current chunk ?
|
This has been pulled into #8650 |
3 participants
@bkietz @romainfrancois something like this would be suitable?
On the R side it would be enough to implement the
Extendmethod for each converter.