Skip to content

Conversation

@rmnskb
Copy link
Contributor

@rmnskb rmnskb commented Jul 19, 2025

Rationale for this change

Please see Github Issue #47123

What changes are included in this PR?

Added public Type Enums that mimic the original private variable groups used for internal type checking.

Are these changes tested?

Yes. Partly for now.

Are there any user-facing changes?

No, just additional features were added: they will now able to access the underlying types directly via the Type Enums.

@rmnskb rmnskb requested review from AlenkaF, raulcd and rok as code owners July 19, 2025 15:47
@github-actions
Copy link

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

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jul 19, 2025
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 19, 2025
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 20, 2025
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 20, 2025
@rmnskb rmnskb requested a review from lidavidm July 20, 2025 11:11
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks!

@lidavidm
Copy link
Member

Actually, run-end-encoded is also missing. As are all the binary types

@rmnskb rmnskb requested review from lidavidm, raulcd and rok July 21, 2025 21:21
Copy link
Member

@AlenkaF AlenkaF left a comment

Choose a reason for hiding this comment

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

All the comments have been addressed. One more suggestion I have is that this enum could be added to the docs. For example to the API doc:

https://github.com/apache/arrow/blob/main/docs/source/python/api/datatypes.rst

next to Type Classes or Utility Functions. Maybe even in the User Guide =) https://github.com/apache/arrow/blob/main/docs/source/python/data.rst

@AlenkaF
Copy link
Member

AlenkaF commented Jul 23, 2025

ps: the linter error is not connected to the changes in this PR. Not sure if there is an issue open already though.

@raulcd
Copy link
Member

raulcd commented Jul 23, 2025

ps: the linter error is not connected to the changes in this PR. Not sure if there is an issue open already though.

The linter issue is solved on main, rebasing main should fix it.

@rmnskb
Copy link
Contributor Author

rmnskb commented Jul 24, 2025

All the comments have been addressed. One more suggestion I have is that this enum could be added to the docs. For example to the API doc:

https://github.com/apache/arrow/blob/main/docs/source/python/api/datatypes.rst

next to Type Classes or Utility Functions. Maybe even in the User Guide =) https://github.com/apache/arrow/blob/main/docs/source/python/data.rst

Thanks for the suggestion! I've put the docs below the Type Checking like so:
image

Partly because the functions above come from the same module, and also because I think this is the intended usage that @lidavidm imagined initially.
Please let me know if there's something else I could help with :)

@github-actions github-actions bot added awaiting merge Awaiting merge awaiting changes Awaiting changes and removed awaiting change review Awaiting change review awaiting merge Awaiting merge labels Jul 24, 2025
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 24, 2025
Copy link
Member

@AlenkaF AlenkaF left a comment

Choose a reason for hiding this comment

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

Thank you for all the updates, this looks good to me!
Will merge once David has one final check.

@AlenkaF
Copy link
Member

AlenkaF commented Jul 25, 2025

@rmnskb one small issue to fix and then we are good to go! =)

______________________ [doctest] pyarrow.types.TypesEnum _______________________
057 
058     Examples
059     --------
060     >>> import pyarrow as pa
061     >>> from pyarrow.types import TypesEnum
062     >>> int8_field = pa.field('int8_field', pa.int8())
063     >>> int8_field.type.id == TypesEnum.INT8
064     True
065 
066     >>> fixed_size_list = pa.list(pa.uint16(), 3)
UNEXPECTED EXCEPTION: AttributeError("module 'pyarrow' has no attribute 'list'")
Traceback (most recent call last):
  File "/opt/conda/envs/arrow/lib/python3.10/doctest.py", line 1350, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest pyarrow.types.TypesEnum[4]>", line 1, in <module>
AttributeError: module 'pyarrow' has no attribute 'list'. Did you mean: 'list_'?

@rmnskb
Copy link
Contributor Author

rmnskb commented Jul 25, 2025

@AlenkaF fixed the typo :)

@AlenkaF
Copy link
Member

AlenkaF commented Jul 25, 2025

Thank you @rmnskb !

@AlenkaF AlenkaF merged commit 2949fe8 into apache:main Jul 25, 2025
14 checks passed
@AlenkaF AlenkaF removed the awaiting change review Awaiting change review label Jul 25, 2025
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 2949fe8.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants