Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARROW-3470: [C++] Fix row-wise example #3078

Closed
wants to merge 2 commits into from
Closed

ARROW-3470: [C++] Fix row-wise example #3078

wants to merge 2 commits into from

Conversation

fsaintjacques
Copy link
Contributor

  • Implement the ADD_EXAMPLE cmake function with new ctest label
    example, also covered by the runexample target. This can be
    toggled via the ARROW_BUILD_EXAMPLES option which is ON by default.
  • Implement fully working row-wise-conversion-example.cc and add it to
    the default build.
  • Update documentation to embed (manually) the newly created example.

@wesm
Copy link
Member

wesm commented Dec 3, 2018

This might be conflicting with #2856. Can we include the C++ file in the generated docs without copying it manually?

@xhochy
Copy link
Member

xhochy commented Dec 3, 2018

We should be able to include a whole file into the Sphinx docs via https://breathe.readthedocs.io/en/latest/directives.html#doxygenfile

I'll merge #2856 as soon as the build passes, so that this PR can rebase on top. We should get rid of the doxygen tutorials completely soon. We may still build the doxygen HTML but only as API docs. All prose documentation should be in Sphinx.

xhochy
xhochy requested changes Dec 3, 2018
View reviewed changes
cpp/apidoc/tutorials/row_wise_conversion.md Outdated
// supported on Unix systems, not Windows.
arrow::MemoryPool* pool = arrow::default_memory_pool();

auto id_builder = std::make_shared<Int64Builder>(pool);
Copy link
Member

Choose a reason for hiding this comment

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

These builders are only used in the local scope. We should make them simple stack variables without wrapping them into std::shared_ptr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrapped them out of uniformity. The cost_components_builder has to be in a shared_ptr to be passed to the components_builder. The previous version was wrongly using a std::move then referencing again the (moved out) builder.

Our choice is between:

  • 3 local and 1 shared_ptr builder
  • 4 shared_ptr builders

I can revert this choice.

@pitrou
Copy link
Member

pitrou commented Dec 4, 2018

If you want to include the source for a file in a Sphinx document, you can use the literalinclude directive: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-literalinclude

@fsaintjacques
Copy link
Contributor Author

Yeah, pre #2856 I was looking at Doxygen documentation to achieve this.

@fsaintjacques
Copy link
Contributor Author

I tend to prefer prose as a single file with prose comments as it allows to copy-paste in a single pass for users. Anyone objects that I go this route? It'll also give CI testing of C++ examples documentation.

@wesm
Copy link
Member

wesm commented Dec 7, 2018

No objections here

@fsaintjacques fsaintjacques force-pushed the ARROW-3470-out-of-date-example branch 5 times, most recently from 0f26ba2 to 602b417 Compare December 10, 2018 19:50
@fsaintjacques
Copy link
Contributor Author

@xhochy that would be ready to review

xhochy
xhochy approved these changes Dec 12, 2018
View reviewed changes
Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

+1, this is looking good but sadly needs a rebase now.

Anyone: Feel free to merge once build passes.

wesm
wesm reviewed Dec 12, 2018
View reviewed changes
Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

I left a few other comments

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/cmake_modules/BuildUtils.cmake Outdated Show resolved Hide resolved
cpp/cmake_modules/BuildUtils.cmake Outdated Show resolved Hide resolved
cpp/examples/arrow/CMakeLists.txt Outdated
# under the License.

#add_executable(row-wise-conversion-example row-wise-conversion.cc)
#target_link_libraries(row-wise-conversion-example arrow)
Copy link
Member

Choose a reason for hiding this comment

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

Cruft

cpp/examples/arrow/row-wise-conversion-example.cc Show resolved Hide resolved
@fsaintjacques
Copy link
Contributor Author

I addressed the comments and replicated what was done to CI scripts with the new tests default to off.

wesm
wesm approved these changes Dec 12, 2018
View reviewed changes
Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1, thanks @fsaintjacques! Will merge once the build runs

pitrou
pitrou reviewed Dec 12, 2018
View reviewed changes
docs/source/cpp/index.rst
@@ -23,3 +23,4 @@ C++ Implementation

getting_started
api
examples
Copy link
Member

Choose a reason for hiding this comment

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

The examples should come before the API IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll let the CI finish, and if it passes I'll push with this tiny change.

@fsaintjacques
ARROW-3470: [C++] Fix row-wise example
1eba067 
- Implement the `ADD_EXAMPLE` cmake function with new ctest label
  `example`, also covered by the `runexample` target. This can be
  toggled via the `ARROW_BUILD_EXAMPLES` option which is ON by default.
- Implement fully working `row-wise-conversion-example.cc` and add it to
  the default build.
- Update documentation to embed (manually) the newly created example.
@fsaintjacques
ARROW-3470: Fix status macro
fab63f6 
When ARROW_EXTRA_ERROR_CONTEXT is enabled and the ARROW_RETURN_NOT_OK
macro is used in a non-arrow namespace, the compiler would throw an
error.
@fsaintjacques
Copy link
Contributor Author

I don't think the failures were related to this change (gandiva micro benchmark), the second one is odd (https://travis-ci.org/apache/arrow/jobs/467085379)

Error message: Invalid: /home/travis/build/apache/arrow/cpp/src/arrow/ipc/json-integration-test.cc:151 code: RecordBatchFileReader::Open(arrow_file.get(), &arrow_reader)
/home/travis/build/apache/arrow/cpp/src/arrow/ipc/reader.cc:624 code: ReadFooter()
File is too small: 0

Concurrency issue?

@wesm
Copy link
Member

wesm commented Dec 12, 2018

I've been seeing this pretty consistently https://travis-ci.org/apache/arrow/jobs/467092615#L2567. I opened https://issues.apache.org/jira/browse/ARROW-4008. It's not related to this patch

@fsaintjacques
Copy link
Contributor Author

I think this is now safe to merge.

@pitrou pitrou closed this in 7ddfba6 Dec 12, 2018
@fsaintjacques fsaintjacques deleted the ARROW-3470-out-of-date-example branch December 18, 2018 12:55
@asfimport asfimport mentioned this pull request Dec 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitrou pitrou pitrou left review comments

@xhochy xhochy xhochy approved these changes

@wesm wesm wesm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fsaintjacques @wesm @xhochy @pitrou