Skip to content

ARROW-9164: [C++] Add embedded documentation to compute functions - #8457

Closed
pitrou wants to merge 2 commits into
apache:masterfrom
pitrou:ARROW-9164-compute-func-docs
Closed

ARROW-9164: [C++] Add embedded documentation to compute functions#8457
pitrou wants to merge 2 commits into
apache:masterfrom
pitrou:ARROW-9164-compute-func-docs

Conversation

@pitrou

@pitrou pitrou commented Oct 13, 2020

Copy link
Copy Markdown
Member

Also re-use the embedded documentations in Python to generate nicer signatures and docstrings.

@pitrou
pitrou requested review from bkietz and wesm and removed request for wesm October 13, 2020 17:14
@github-actions

Copy link
Copy Markdown

@pitrou
pitrou force-pushed the ARROW-9164-compute-func-docs branch 2 times, most recently from 2c75b60 to 8010c4e Compare October 13, 2020 18:10
Comment thread python/pyarrow/compute.py Outdated

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.

Wouldn't it be better to set the __signature__ property instead?

@bkietz bkietz left a comment

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.

Altogether this looks like a great improvement in the usability of the compute module, thanks for doing this!

Comment thread cpp/src/arrow/compute/function.h Outdated
Comment thread python/pyarrow/compute.py Outdated
Comment thread python/pyarrow/compute.py Outdated
Comment thread python/pyarrow/compute.py Outdated
Comment thread python/pyarrow/compute.py Outdated
Comment thread python/pyarrow/compute.py Outdated

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.

Suggested change
if option_class is None:
def wrapper(*args, *, memory_pool=None):
return func.call(args, None, memory_pool)
wrapped_template = 'lambda {}, *, memory_pool=None: ()'
else:
def wrapper(*args, *, options=None, memory_pool=None, **kwargs):
options = _handle_options(name, option_class, options, kwargs)
return func.call(args, options, memory_pool)
wrapped_template = 'lambda {}, *, options=None, memory_pool=None, **kwargs: ()'
wrapper.__name__ = name
wrapper.__qualname__ = name
wrapper.__wrapped__ = eval(wrapped_template.format(args_sig))

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.

Hmm, I didn't know that __wrapped__ was taken up by inspect.signature. Interesting. I'd rather examine this later though.

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.

@jorisvandenbossche jorisvandenbossche left a comment

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.

This looks great! Also the python docstrings ;)

I mostly did a review of the docstrings' content now (and not yet the generation machinery)

Comment thread cpp/src/arrow/compute/kernels/aggregate_var_std.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/scalar_boolean.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/scalar_boolean.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/scalar_validity.cc Outdated

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.

Not for this PR, but I think that for conceptually more complex functions (like this one, for me), it might be useful to include a small example. But that might get verbose to write in this form ..

For example, for this case: [[1, 2], [3, 4, 5], [6]] returns [0, 0, 1, 1, 1, 2]

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.

Yes, it's not obvious how to make this too unwiedly to write.

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.

Going further down the docstring rabbithole: we could introduce a doctest-like class (of which a FunctionDoc contains a vector), which could just be a set of lambdas which generate a set of inputs, options, and maybe the expected output. These can then be rendered lazily in python

Comment thread cpp/src/arrow/compute/kernels/vector_sort.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/vector_sort.cc Outdated
@pitrou
pitrou force-pushed the ARROW-9164-compute-func-docs branch from 8010c4e to 2b0e393 Compare October 15, 2020 16:27
@pitrou

pitrou commented Oct 15, 2020

Copy link
Copy Markdown
Member Author

@nealrichardson @kou This may help for R and Ruby.

@kou

kou commented Oct 16, 2020

Copy link
Copy Markdown
Member

Wow! Great!
I want not only argument name but also acceptable argument types to generate methods automatically in Ruby. (It'll be a follow-up task.)
For example, if the first argument of function A (x of A(x, y, z)) accepts a table and a record batch, we can add an A method to table and record batch classes:

x = table
x.A(y, z)

x = record_batch
x.A(y, z)

@pitrou

pitrou commented Oct 19, 2020

Copy link
Copy Markdown
Member Author

Will rebase and merge.

Also re-use the embedded documentations in Python to generate nicer signatures and docstrings.
Expose CountOptions.

Improve introspectability of option classes.
@pitrou
pitrou force-pushed the ARROW-9164-compute-func-docs branch from 2b0e393 to 61fc098 Compare October 19, 2020 12:00
@pitrou

pitrou commented Oct 19, 2020

Copy link
Copy Markdown
Member Author

Travis-CI build: https://travis-ci.org/github/pitrou/arrow/builds/737045923

CI failures are unrelated.

@pitrou pitrou closed this in 69b444b Oct 19, 2020
@pitrou
pitrou deleted the ARROW-9164-compute-func-docs branch October 19, 2020 12:56
kszucs pushed a commit that referenced this pull request Oct 19, 2020
Also re-use the embedded documentations in Python to generate nicer signatures and docstrings.

Closes #8457 from pitrou/ARROW-9164-compute-func-docs

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
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.

5 participants