GH-33377: [Python] Table.drop should support passing a single column

[danepitkin]

Rationale for this change

Provide a better user experience in pyarrow when working with Table.

What changes are included in this PR?

Allow Table.drop() to accept a single column name as a str argument.
Provide a wrapper Table.drop_column(str), which calls Table.drop(), to match similar APIs such as add_column(), append_column().

Are these changes tested?

Updated the pytest for Table.drop() and added a pytest for Table.drop_column(). Verified both test cases ran successfully locally.

Are there any user-facing changes?

Yes, a new pyarrow API is added. The existing pyarrow API is backwards compatible, but does support additional types of input now (str). The doc strings are updated so I assume the python API reference will be auto-updated somehow? Let me know if this is not the case.

• Closes: [Python] Table.drop should support passing a single column #33377

[github-actions]

• Closes: [Python] Table.drop should support passing a single column #33377

[github-actions]

⚠️ GitHub issue #33377 has been automatically assigned in GitHub to PR creator.

[danepitkin]

This is my first Arrow PR. I am very open to any feedback or suggestions you might have!

I followed the instructions as stated in the original GH issue, but there is now some redundancy between Table.drop(<str or list(str)>), Table.drop_column(<str>), and Table.remove_column(<index>). In my opinion, it would have been nicer to consolidate these from the start. However, since backwards compatibility is important, would you say this PR is the right approach?

[rok]

This is my first Arrow PR. I am very open to any feedback or suggestions you might have!

Welcome @danepitkin! Thanks for opening the PR!

I followed the instructions as stated in the original GH issue, but there is now some redundancy between Table.drop(<str or list(str)>), Table.drop_column(<str>), and Table.remove_column(<index>). In my opinion, it would have been nicer to consolidate these from the start. However, since backwards compatibility is important, would you say this PR is the right approach?

I would slightly prefer Table.drop_column(<str or list(str)>) over Table.drop_column(<str>) and Table.drop(<list(str)>). I don't know if .drop -> .drop_column makes sense at this point. Pandas uses .drop for everything but it does index dropping as well. Perhaps you could just alias the two and introduce a deprecation warning? (e.g. here).

[danepitkin]

Thanks, Rok!

I would slightly prefer Table.drop_column(<str or list(str)>) over Table.drop_column(<str>) and Table.drop(<list(str)>). I don't know if .drop -> .drop_column makes sense at this point. Pandas uses .drop for everything but it does index dropping as well. Perhaps you could just alias the two and introduce a deprecation warning? (e.g. here).

I like this plan. Also, I think this is what the original issue was requesting and I just misread it! I'll update the ticket with Table.drop_column(<str or list(str)>) and mark Table.drop() with a deprecation warning. While Arrow does append the word _column to other Table modification APIs (e.g. add_column in arrow, add in pandas), I'd be hesitant to remove Table.drop() anytime soon, if ever. Breaking backwards compatibility for minor API UX improvements goes against providing a good UX in general. Just wanted to document my thinking here. Overall, I am aligned with you!

[rok]

I'd be hesitant to remove Table.drop() anytime soon, if ever. Breaking backwards compatibility for minor API UX improvements goes against providing a good UX in general.

Indeed. I think it also depends a bit on the capability evolution - if we ever introduce .drop_rows it would make much more sense to deprecate .drop than it does now.

[danepitkin]

I decided to name it Table.drop_columns instead of Table.drop_column to better communicate the possible actions.

Also, instead of a wrapper with a warning, what if we alias it directly with drop = drop_columns? I'm not sure if we can still add the warning anymore, but maybe we don't need to officially deprecate the API if there is no timeline for removal.

[jorisvandenbossche]

Also, instead of a wrapper with a warning, what if we alias it directly with drop = drop_columns? I'm not sure if we can still add the warning anymore, but maybe we don't need to officially deprecate the API if there is no timeline for removal.

I don't have a strong opinion, but if we don't actually plan to remove it in a few releases, we can indeed also just alias it as you said without warning.

[jorisvandenbossche]

Looks good!

[jorisvandenbossche]

Either way if we just keep it as alias or actively deprecate it, I think it's also fine to only keep the docstring up to here, saying it is an alias, and then refer to the docstring of drop_columns for more details. That avoids duplicating the rest of the content.

[danepitkin]

Thanks! I tried this at first, but thought I was getting a document build error. I probably was just building it wrong 😬 haha. I'll do this because I very much dislike duplication.

[jorisvandenbossche]

Suggested change

	"Table.drop is deprecated, use Table.drop_columns.",
	DeprecationWarning)
	"Table.drop is deprecated, use Table.drop_columns.",
	DeprecationWarning, stacklevel=2)

(to ensure the warning points to where the method is actually used and not to this location)

[jorisvandenbossche]

If we keep the warning, we can assert that this raises a warning (pytest.raises), which at the same time also suppresses the warning (we don't want that running our test suite prints this expected warning)

[danepitkin]

Thank you! I'm going to remove the warning since there is no plan for deprecation. Good to know for the future though!

[jorisvandenbossche]

Is this docstring for sphinx? (interactively checking the docstring in a terminal/notebook, that still gives the drop_columns one)

[danepitkin]

Great catch!! Will fix this.

[danepitkin]

I'm going to remove the alias and use a wrapper. I'm not sure if it's possible to update the docstring of an alias without affecting the original method's docstring.

I tried this:

    drop = drop_columns
    drop.__doc__ = """Alias of Table.drop_columns, kept for backwards compatibility."""

..but got an error:

Traceback (most recent call last):
  ...
  File "pyarrow/table.pxi", line 4729, in init pyarrow.lib
AttributeError: attribute '__doc__' of 'method_descriptor' objects is not writable

[jorisvandenbossche]

I suppose that's because it's implemented in cython, and then that attribute is not writeable as it would be in python (just a guess, didn't actually verify)

But anyway, the wrapper looks good!

[jorisvandenbossche]

Thanks!

[rok]

Congrats @danepitkin !

[ursabot]

Benchmark runs are scheduled for baseline = 177a0d4 and contender = 36d3394. 36d3394 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.24% ⬆️0.12%] test-mac-arm
[Finished ⬇️5.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.06%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 36d3394c ec2-t3-xlarge-us-east-2
[Failed] 36d3394c test-mac-arm
[Finished] 36d3394c ursa-i9-9960x
[Finished] 36d3394c ursa-thinkcentre-m75q
[Finished] 177a0d44 ec2-t3-xlarge-us-east-2
[Finished] 177a0d44 test-mac-arm
[Finished] 177a0d44 ursa-i9-9960x
[Finished] 177a0d44 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[ursabot]

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x