ARROW-9644: [C++][Dataset] Don't apply ignore_prefixes to partition base_dir

[bkietz]

I still apply ignore_prefixes to all segments of paths yielded by a selector which lie outside an explicit partition base directory.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-9644

[jorisvandenbossche]

And the "partition base directory" is automatically set if a user does something like ds.dataset("_shouldnt_be_ignored/dataset/") ?

[bkietz]

And the "partition base directory" is automatically set if a user does something like ds.dataset("_shouldnt_be_ignored/dataset/") ?

Currently in python the partition_base_dir is always identical to the base directory of the recursive selector used, so yes.

In principle it would be possible to select /mnt/data/** and set partition_base_dir=/mnt/data/partitioned, in which case /mnt/data/other/** would be included in the dataset but would not have partition information attached. Alternatively, one could set partition_base_dir=/mnt and select /mnt/year=2020/**, in which case all fragments would include the "year"_ == 2020 partition expression. I question the utility of these cases, honestly.

[bkietz]

(again, moot for python since the two directories are identical) Since the user's selection of paths is really based in selector.base_dir, I think it'd be more approprate to apply ignore_prefixes relative to that

[jorisvandenbossche]

Added the specific case of a directory to the existing test about this

[nealrichardson]

I didn't understand what the Appveyor failure was so I re-triggered the build to see if it was a flake.

[nealrichardson]

Segfault in test_dataset.py?

============================= test session starts =============================
platform win32 -- Python 3.7.8, pytest-6.0.1, py-1.9.0, pluggy-0.13.1
rootdir: C:\projects\arrow\python, configfile: setup.cfg
plugins: hypothesis-5.24.0, lazy-fixture-0.6.3
collected 3415 items / 3 skipped / 3412 selected
pyarrow\tests\test_adhoc_memory_leak.py s                                [  0%]
pyarrow\tests\test_array.py ......................s..................... [  1%]
.....................................................................sss [  3%]
sssssss...............................................................ss [  5%]
.......                                                                  [  5%]
pyarrow\tests\test_builder.py ....                                       [  5%]
pyarrow\tests\test_cffi.py ....                                          [  5%]
pyarrow\tests\test_compute.py .......................................... [  7%]
.................................................................        [  9%]
pyarrow\tests\test_convert_builtin.py .................................. [ 10%]
........................................................................ [ 12%]
........................................................................ [ 14%]
........................................................................ [ 16%]
............................x........................................... [ 18%]
.......x....ssss......................................................   [ 20%]
pyarrow\tests\test_csv.py .............................................. [ 21%]
..........................s.                                             [ 22%]
pyarrow\tests\test_cython.py .                                           [ 22%]
pyarrow\tests\test_dataset.py ....................
(arrow) C:\projects\arrow\python>set lastexitcode=3 
(arrow) C:\projects\arrow\python>set  1>C:\Users\appveyor\AppData\Local\Temp\1\tmp728E.tmp 
(arrow) C:\projects\arrow\python>echo C:\projects\arrow\python  1>C:\Users\appveyor\AppData\Local\Temp\1\tmp728F.tmp 
(arrow) C:\projects\arrow\python>exit /b 3 
Command exited with code 3

[pitrou]

I'm taking a look at the segfault.

[pitrou]

@jorisvandenbossche This seems to work for now. I'm not sure if fsspec normalizes paths internally (especially on Windows).

[jorisvandenbossche]

From a quick looks, it seems that their LocalFileSystem should handle incoming "windows" (non-normalized) paths (see eg https://github.com/intake/filesystem_spec/pull/65/files, where a bunch of path = make_path_posix(path) were added to the LocalFileSystem methods). So I think this should be fine.

Only, do we rely on this method on the C++ side to actually return a normalized path?
Eg in the C++ dataset discovery code, that might be relying on being able to split the normalized path on / ?

[pitrou]

When calling GetFileInfo(selector), we need the return values to be a lexical child of the selector base_dir.

[pitrou]

So if GetFileInfo normalizes, so should NormalizePath. Otherwise, not necessary.

[pitrou]

That said, we can also special-case fsspec's LocalFileSystem to use our own LocalFileSystem instead... do we already do that?

[jorisvandenbossche]

We indeed already do that:

arrow/python/pyarrow/fs.py

Lines 77 to 79 in 90d1ab7

	if type(filesystem).__name__ == 'LocalFileSystem':
	# In case its a simple LocalFileSystem, use native arrow one
	return LocalFileSystem(use_mmap=use_mmap)

at least in cases where the _ensure_filesystem is used to process the user specified filesystem before passing it to the underlying functions (but which should in principle be done by all user facing functions).

So the actual fsspec's LocalFileSystem is only used for testing, so I think this is certainly fine then (since other filesystems should not have this problem of windows paths)?

[pitrou]

Let's assume it's fine, then.