Skip to content

ARROW-3475: [C++] Allow builders to finish to the corresponding array type#4262

Closed
bkietz wants to merge 5 commits intoapache:masterfrom
bkietz:3475-Int64Builder-FinishNumericArrayInt64Type
Closed

ARROW-3475: [C++] Allow builders to finish to the corresponding array type#4262
bkietz wants to merge 5 commits intoapache:masterfrom
bkietz:3475-Int64Builder-FinishNumericArrayInt64Type

Conversation

@bkietz
Copy link
Copy Markdown
Member

@bkietz bkietz commented May 6, 2019

Allows Int64Builder to finish into shared_ptr<Int64Array>, ListBuilder to finish into shared_ptr<ListArray>, etc.

@bkietz
Copy link
Copy Markdown
Member Author

bkietz commented May 6, 2019

Hmmm, I'm not sure how to resolve this doc error. Doxygen produces an extra declaration of ArrayBuilder::Finish from the explicit using. Without the explicit using, the base class' overload for plain Array doesn't participate in overload resolution.

I could rename the method to FinishTyped but that seems like a sloppy solution. Is there a doxygen tag/option I could use to explicitly ignore the declaration from using?

ARROW_RETURN_NOT_OK(Finish(&out_untyped));
*out = std::static_pointer_cast<ArrayType>(std::move(out_untyped));
return Status::OK();
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option is to Call this Finish simply and add a DCHECK to assert that the runtime type matches ArrayType. I tried this here:

wesm@0be7cda

I'm not sure what pitfalls there might be from that

Copy link
Copy Markdown
Member Author

@bkietz bkietz May 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an option. I avoided it because it seems sloppy/user-hostile to allow something to compile which could easily be caught as an error:

Int64Builder i64_b;
// ...
std::shared_ptr<Int32Array> i32_a;
i64_b.Finish(&i32_a); // would compile, but fail at runtime

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way would be to make this a non-member method:

Int64Builder builder;
// ...
std::shared_ptr<Int64Array> array;
Finish(&builder, &array); // type match guaranteed at compile time

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Well, I think it's OK to accept things as they are now, the boilerplate does not seem too onerous

@wesm
Copy link
Copy Markdown
Member

wesm commented May 7, 2019

+1

@wesm wesm closed this in 0a5f90a May 7, 2019
@bkietz bkietz deleted the 3475-Int64Builder-FinishNumericArrayInt64Type branch February 25, 2021 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants