COMPAT: reading generic PyTables Table format fails with sub-selection
State: closed

- [x] closes #11188
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Bug
name: IO HDF5
description: read_hdf, HDFStore

Comments:
Hello @jgehrcke! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:







There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-06-21 13:33:33 UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26818?src=pr&el=h1) Report
> Merging [#26818](https://codecov.io/gh/pandas-dev/pandas/pull/26818?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/634577eb9ae53da6c16a54773f9f9f8016e2a1ed?src=pr&el=desc) will **decrease** coverage by `50.76%`.
> The diff coverage is `75%`.

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

```diff
@@             Coverage Diff             @@
##           master   #26818       +/-   ##
===========================================
- Coverage   91.86%    41.1%   -50.77%     
===========================================
  Files         179      179               
  Lines       50707    50710        +3     
===========================================
- Hits        46583    20842    -25741     
- Misses       4124    29868    +25744
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `41.1% <75%> (-0.1%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26818?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/pytables.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3B5dGFibGVzLnB5) | `89.3% <75%> (-1%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/plotting/\_matplotlib/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19tYXRwbG90bGliL19faW5pdF9fLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/gcs.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2djcy5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/s3.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3MzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.37%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.16%)` | :arrow_down: |
| ... and [133 more](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26818?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/26818?src=pr&el=footer). Last update [634577e...7f21dbe](https://codecov.io/gh/pandas-dev/pandas/pull/26818?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26818?src=pr&el=h1) Report
> Merging [#26818](https://codecov.io/gh/pandas-dev/pandas/pull/26818?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/224362951942e1f4e05fb8948596620aedac26d9?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #26818      +/-   ##
==========================================
- Coverage   91.98%   91.98%   -0.01%     
==========================================
  Files         180      180              
  Lines       50772    50774       +2     
==========================================
- Hits        46704    46702       -2     
- Misses       4068     4072       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.62% <100%> (+0.04%)` | :arrow_up: |
| #single | `41.82% <100%> (-0.09%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26818?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/pytables.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3B5dGFibGVzLnB5) | `90.3% <100%> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `88.88% <0%> (-11.12%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26818/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.89% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26818?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/26818?src=pr&el=footer). Last update [2243629...1ce1a70](https://codecov.io/gh/pandas-dev/pandas/pull/26818?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Funny `codecov` bot :-). Well, let's wait for this round of CI to report back. 
@gfyoung @simonjayhawkins @jreback I would appreciate your feedback, especially about the "fix" itself for starters. The workaround is not necessarily nice. All tests pass, but could it be that this approach (introducing `table._nrows_to_read = stop - start` dynamically, and using it dynamically instead of `table.nrows`) has negative side effects? I want to say that I didn't quite see a meaningful way to pass `start` and `stop` explicitly down to `GenericIndexCol`; by the moment we talk about `GenericIndexCol` these two parameters are more table parameters than index col parameters. WDYT?
@gfyoung @jreback after following the quite entangled code path I think I found a cleaner solution.

Key insight was the following:

In `HDFStore.select()` we define a callback `func`:
```python
        # function to call on iteration
        def func(_start, _stop, _where):
            return s.read(start=_start, stop=_stop,
                          where=_where,
                          columns=columns)

        # create the iterator
        it = TableIterator(self, s, func, where=where, nrows=s.nrows,
                           start=start, stop=stop, iterator=iterator,
                           chunksize=chunksize, auto_close=auto_close)

        return it.get_result()
```

In the problematic code path `s.read()` actually translates to `Table.read_axes()` which is prepared to receive arbitrary keyword arguments:

```python
    def read_axes(self, where, **kwargs):
        """create and return the axes sniffed from the table: return boolean
        for success
        """
        ...
```

As of the callback being `s.read(start=_start, stop=_stop, ....)` we know that the `start` and `stop` parameters are available as keyword arguments within `Table.read_axes()`.

`read_axes()` calls into `GenericIndexCol.convert()` and so this is a decent opportunity to pass the `start` and `stop` parameters directly to `convert()`. I have added a corresponding commit. Please have another look, thanks!
Will polish (squash commits, address line length issues) after another round of comments.

Update: just force-pushed after rebase on current master:

- removed the unrelated requirements-dev.txt patch
- addressed linter errors (line length)
@jreback I tried to address all points in your last round of feedback. I feel like we're getting closer. Would very much appreciate another look, thanks for your time :clock10: !
@jreback thanks for your feedback once again.

I have
- rebased on upstream/master (clean)
- added four new commits to address your last comments

In particular, I have now moved the new tests to `pandas/tests/io/pytables/test_compat.py` as suggested. While doing so I have not created a  `pandas/tests/io/pytables/__init__.py` file because I don't see the reason yet (it's not for test discovery, but is it for linters?).

I have also **not** used `ensure_clean_path` in the new test module (because then I would have needed to refactor it out of where it is right now). Instead I have used the pytest `tmp_path` fixture which I think provides everything we need with less code: it guarantees file system isolation between tests, it retains files for later inspection (as opposed to the `ensure_clean_path` context manager) and removes them in a rolling fashion so that storage space requirement stays predictable (does not grow indefinitely with the number of invocations). For instance, this is after invoking pytest 11 times (files from the last three invocations are retained, the rest got rotated away):

```
$ tree
.
├── pytest-10
│   ├── test_read_complete0
│   │   └── written_with_pytables.h5
│   ├── test_read_completecurrent -> /tmp/pytest-of-jp/pytest-10/test_read_complete0
│   ├── test_read_with_start0
│   │   └── written_with_pytables.h5
│   ├── test_read_with_startcurrent -> /tmp/pytest-of-jp/pytest-10/test_read_with_start0
│   ├── test_read_with_startstop0
│   │   └── written_with_pytables.h5
│   ├── test_read_with_startstopcurrent -> /tmp/pytest-of-jp/pytest-10/test_read_with_startstop0
│   ├── test_read_with_stop0
│   │   └── written_with_pytables.h5
│   └── test_read_with_stopcurrent -> /tmp/pytest-of-jp/pytest-10/test_read_with_stop0
├── pytest-8
│   ├── test_read_complete0
│   │   └── written_with_pytables.h5
│   ├── test_read_completecurrent -> /tmp/pytest-of-jp/pytest-8/test_read_complete0
│   ├── test_read_with_start0
│   │   └── written_with_pytables.h5
│   ├── test_read_with_startcurrent -> /tmp/pytest-of-jp/pytest-8/test_read_with_start0
│   ├── test_read_with_startstop0
│   │   └── written_with_pytables.h5
│   ├── test_read_with_startstopcurrent -> /tmp/pytest-of-jp/pytest-8/test_read_with_startstop0
│   ├── test_read_with_stop0
│   │   └── written_with_pytables.h5
│   └── test_read_with_stopcurrent -> /tmp/pytest-of-jp/pytest-8/test_read_with_stop0
├── pytest-9
│   ├── test_read_complete0
│   │   └── written_with_pytables.h5
│   ├── test_read_completecurrent -> /tmp/pytest-of-jp/pytest-9/test_read_complete0
│   ├── test_read_with_start0
│   │   └── written_with_pytables.h5
│   ├── test_read_with_startcurrent -> /tmp/pytest-of-jp/pytest-9/test_read_with_start0
│   ├── test_read_with_startstop0
│   │   └── written_with_pytables.h5
│   ├── test_read_with_startstopcurrent -> /tmp/pytest-of-jp/pytest-9/test_read_with_startstop0
│   ├── test_read_with_stop0
│   │   └── written_with_pytables.h5
│   └── test_read_with_stopcurrent -> /tmp/pytest-of-jp/pytest-9/test_read_with_stop0
└── pytest-current -> /tmp/pytest-of-jp/pytest-10
```
@jreback it's green now. Wanna have another look before I squash some commits?
thanks @jgehrcke nice patch. We squash on merge anyhow, so no need!
If interested would like to move the existing test_pytables test files (test_pytables.py and test_pytables_missing.py) to pandas/test/io/pytables (just a simple move). We want to split the main file, but that will be separate.

and I think we *usually* add an ```__init__.py``` file in the pytables/ dir as well
> thanks @jgehrcke nice patch

Thank you for the nice collaboration!
> If interested would like to move the existing test_pytables test files (test_pytables.py and test_pytables_missing.py) to pandas/test/io/pytables (just a simple move). 

Sure, PR upcoming.

Update: https://github.com/pandas-dev/pandas/pull/26986