Skip to content

IGNITE-15266 Add isclass check to is_hinted#48

Closed
bojidar-bg wants to merge 1 commit intoapache:masterfrom
obecto:ignite-15266
Closed

IGNITE-15266 Add isclass check to is_hinted#48
bojidar-bg wants to merge 1 commit intoapache:masterfrom
obecto:ignite-15266

Conversation

@bojidar-bg
Copy link
Copy Markdown
Contributor

The issue seems to be caused by issubclass requiring its first parameter to be a class, which isn't the case for two-element tuples like the ones used by e.g. object arrays for hinting the object array type ((-1, [...])). Hence, an obvious patch would be to just double check that we are using it with a class.

Performance impact:

$ python -m timeit -r 25 -s "from inspect import isclass; a = int;" -- "isclass(a)"
2000000 loops, best of 25: 133 nsec per loop
$ python -m timeit -r 25 -s "from inspect import isclass; a = int;" -- "isclass(a) and issubclass(a, float)"
1000000 loops, best of 25: 236 nsec per loop
$ python -m timeit -r 25 -s "from inspect import isclass; a = [];" -- "isclass(a) and issubclass(a, float)"
2000000 loops, best of 25: 158 nsec per loop
$ python -m timeit -r 25 -s "a = int;" -- "try:" "  issubclass(a, float)" "except:" "  pass"
2000000 loops, best of 25: 114 nsec per loop
$ python -m timeit -r 25 -s "a = [];" -- "try:" "  issubclass(a, float)" "except:" "  pass"
1000000 loops, best of 25: 333 nsec per loop

Doesn't seem to impactful to me, we can do a million of those in a millisecond.

@ivandasch
Copy link
Copy Markdown
Contributor

@bojidar-bg Great catch, Bojidar. Could you please add a test case for it? I've really appreciate your effort and create some tests for your case, please add tests to your patch. Also, we have already imported inspect here, please use inspect.isclass.
Or you can just use my patch. Apply it and commit to your branch. After it, I will merge you PR. Thank you!

Also I'd suggest not to use OBJECT_ARRAY at all, Ignite itself doesn't have a great support of it. Use Collections (simple list) instead.
Add_tests_to_patch.txt

Co-authored-by: Ivan Daschinsky <ivandasch@apache.org>
@bojidar-bg
Copy link
Copy Markdown
Contributor Author

Ah, good spot, didn't notice it was already imported! Thanks for the patch, just went ahead and used it ☺

@ivandasch
Copy link
Copy Markdown
Contributor

@bojidar-bg Great, I'll merge a fix today. I cannot give your a promise to release 0.5.2 as ASAP, unfortunatelly. But let's wait for a while, may be others bugs appears, so we can ship a new bugfix release.

@ivandasch ivandasch closed this in be18440 Aug 6, 2021
@bojidar-bg bojidar-bg deleted the ignite-15266 branch August 6, 2021 11:34
ivandasch pushed a commit that referenced this pull request Sep 9, 2021
Signed-off-by: Ivan Daschinsky <ivandasch@apache.org>
isapego added a commit that referenced this pull request Nov 12, 2024
Signed-off-by: Ivan Daschinsky <ivandasch@apache.org>
(cherry picked from commit fa364df)
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.

2 participants