Accept constant memoryviews in HashTable.lookup
State: closed

xref https://github.com/pandas-dev/pandas/pull/10070
xref https://github.com/pandas-dev/pandas/issues/12813

 building an ExtensionArray for nullable ints using a `pyarrow.Array`, the underlying memoryview is marked as constant. The HashTable Cython code uses the memoryview in the `lookup` function but in a read-only fashion. Without the `const` specifier Cython raises an error that the memoryview is read-only.

Not sure how to test this in a small scope (i.e. without using `pyarrow`) or if this small change is worth mentioning in the changelog.

- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff
name: Dependencies
description: Required and optional dependencies

Comments:
The Travis failure seems quite silent. Is there a way to extract more information?
I restarted the one. I haven't seen that error before.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21688?src=pr&el=h1) Report
> Merging [#21688](https://codecov.io/gh/pandas-dev/pandas/pull/21688?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/e84cf2ad20601dc2da58390f36eb96ca0c902bee?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/21688/graphs/tree.svg?width=650&src=pr&token=eZ4WkYLtcO&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/21688?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #21688      +/-   ##
==========================================
- Coverage   91.95%   91.95%   -0.01%     
==========================================
  Files         160      160              
  Lines       49837    49820      -17     
==========================================
- Hits        45830    45812      -18     
- Misses       4007     4008       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.33% <ø> (-0.01%)` | :arrow_down: |
| #single | `42.07% <ø> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/21688?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/category.py](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9jYXRlZ29yeS5weQ==) | `97.01% <0%> (-0.28%)` | :arrow_down: |
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `94.38% <0%> (-0.25%)` | :arrow_down: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `93.35% <0%> (-0.23%)` | :arrow_down: |
| [pandas/core/indexes/multi.py](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9tdWx0aS5weQ==) | `94.96% <0%> (-0.09%)` | :arrow_down: |
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `92.65% <0%> (-0.05%)` | :arrow_down: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `91% <0%> (-0.03%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.19% <0%> (-0.01%)` | :arrow_down: |
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `98.63% <0%> (ø)` | :arrow_up: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `100% <0%> (ø)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `85.33% <0%> (ø)` | :arrow_up: |
| ... and [6 more](https://codecov.io/gh/pandas-dev/pandas/pull/21688/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21688?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21688?src=pr&el=footer). Last update [e84cf2a...a4cb629](https://codecov.io/gh/pandas-dev/pandas/pull/21688?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Must have been a one-off failure.

cc @chris-b1 if you have any thoughts on this?
Now that cython supports const memoryviews we probably should go ahead and use them everywhere possible, #18825 example of working around this.

Looks like would bump our required version up to `0.28` but shouldn't really be a problem since we distribute c files on PyPI?
https://github.com/cython/cython/blob/master/CHANGES.rst#028-2018-03-13
> is this an actual change in cython?

Yes, it is a new cython feature that it can now work on read-only memoryviews (see the third bullet point here: https://github.com/cython/cython/blob/master/CHANGES.rst#028-2018-03-13). This is also a feature much wanted by sklearn: https://github.com/scikit-learn/scikit-learn/issues/10624

As @chris-b1 said, if we bump the cython version requirement, we should try to simplify our code base and use memoryviews in more places (https://github.com/pandas-dev/pandas/pull/10070 as another example where two version of a function are added, one with memoryviews and other with ndarray interface to support read-only arrays)

IMO, ideally, *if* we bump the version, it would be good to test for a few internal cases that the const memoryviews are actually working (since this is new functionality in cython, there might be still some rough edges. In sklearn they found a bug in const memoryviews that was fixed in 0.28.2). 
(but since I am not going to do that work, no strong opinion on doing this before actually bumping the version requirement)
> Looks like would bump our required version up to 0.28 but shouldn't really be a problem since we distribute c files on PyPI?

That's correct.
Happy to implement the necessary changes when we bump the Cython version but it would be nice to know whether this is the preferred way forward?
yeah am ok with bumping the cython version i guess for 0.24

since this is not user facing really

we should also again bump numpy min but that’s a different issue
after we merge this can try to remove the other read-only memory fixes that are in take routines 
xrref #10070 and #12813 

pushed some small doc changes. ping on green for merge.
thanks @xhochy nice patch!