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.

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

ARROW-5563: [Format] Update integration test JSON format documentation #6377

Closed

Conversation

nealrichardson
Copy link
Contributor

@nealrichardson nealrichardson commented Feb 7, 2020

This fills in details about all types (AFAIK) and adds a couple of examples.

@github-actions
Copy link

github-actions bot commented Feb 7, 2020

docs/source/format/Integration.rst Show resolved Hide resolved
docs/source/format/Integration.rst Outdated Show resolved Hide resolved
docs/source/format/Integration.rst Outdated Show resolved Hide resolved
nealrichardson and others added 2 commits March 5, 2020 13:48
Co-Authored-By: Benjamin Kietzman <bengilgit@gmail.com>
@nealrichardson nealrichardson marked this pull request as ready for review March 5, 2020 21:52
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.

Thanks for doing this. minor comments

docs/source/format/Integration.rst Outdated Show resolved Hide resolved
docs/source/format/Integration.rst Show resolved Hide resolved
docs/source/format/Integration.rst Show resolved Hide resolved
"count" "field_length",
"$BUFFER_TYPE": /* BufferData */
...
"$BUFFER_TYPE": /* BufferData */
Copy link
Member

Choose a reason for hiding this comment

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

For the record, it might be preferable to instead have a "buffers": [BufferData]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe so though that's out of scope here since I'm documenting as-built.

docs/source/format/Integration.rst Show resolved Hide resolved
integration/README.md Show resolved Hide resolved
@nealrichardson
Copy link
Contributor Author

Feedback addressed, PTAL again @bkietz @wesm

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 here. Thanks @nealrichardson

@wesm wesm closed this in 81176c2 Mar 6, 2020
@nealrichardson nealrichardson deleted the integration-test-docs branch March 6, 2020 04:56
The integration test data generator and runner uses ``archery``, a Python script
that requires Python 3.6 or higher. You can create a standalone Python
distribution and environment for running the tests by using
`miniconda <https://conda.io/miniconda.html>`_. On Linux this is:
Copy link
Member

Choose a reason for hiding this comment

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

Do we really have to repeat the Miniconda install instructions on every other documentation page? That isn't scalable. I suggest to either let the user figure it out (it isn't difficult) or to put those in a dedicated page (development FAQ perhaps?).

refer to it if you have questions about how to build other languages or enable
certain tests.

See :ref:`integration` for more information about the project's
Copy link
Member

Choose a reason for hiding this comment

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

It's a pity that other page is also called "integration"... that's confusing. Perhaps rename it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://issues.apache.org/jira/browse/ARROW-7122 is the issue for improving the docker-compose documentation: we should fix this there.

@@ -63,7 +53,10 @@ such topics as:

cpp/index
python/index
java/index
`Java <https://arrow.apache.org/docs/java/>`_
Copy link
Member

@pitrou pitrou Apr 14, 2020

Choose a reason for hiding this comment

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

It looks like this syntax breaks the side bar (did you test?). See screenshot below:
https://framapic.org/gYWKmMs30com/kHYNqQJCHhf7.png

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.

None yet

4 participants