It is very hard to review this PR due to the way the diff is presented in GitHub. I tried to summarise the main changes in the description of the PR, hope it helps a bit.

@jorisvandenbossche after the last review I have updated the marks in the tests, added use_legacy_dataset=None to ParquetDataset class, read_table and arite_to_dataset.

I have also removed ** kwargs from ParquetDataset, previously _ParquetDatasetV2, which meant I had to remove the code connected to metadata, split_row_groups and validate_schema (raising an error) and so I added a note in the docstrings b6799cf. Not sure if that is well done though.

The HDFS failures seem to be related: https://github.com/ursacomputing/crossbow/actions/runs/7288206604/job/19860362598#step:6:9473

Yeah, missed some metadata there. Thanks, will correct!

@jorisvandenbossche if I am understanding correctly read_parquet from the failed build is part of a legacy Filesystem and will be removed? So I can for time being add a ValueError if metadata is specified or I can just remove it from the signature?

@github-actions crossbow submit -g python-*-hdfs

@github-actions crossbow submit hdfs

I am not sure if raising for the metadata will be sufficient? My understanding is that those tests (for the legacy HDFS filesystem) are using hdfs.read_parquet -> pq.read_table by passing a legacy HDFS filesystem object, and with removing the legacy parquet code, we no longer support the legacy filesystems (and now as a next step, we can actually also remove those legacy filesystems).
So maybe we can just "xfail" those tests for now? And then when removing the HDFS legacy filesystem, we can see if we just remove those tests, or if we rewrite them using the new hdfs filesystem.

Revision: 77b4ecb

Submitted crossbow builds: ursacomputing/crossbow @ actions-bb763d7a57

Task	Status
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1

Still need to look into the failures with Unrecognized filesystem error.

Ah, I guess the issue is the same it only fails in _ensure_filesystem and not in read_parquet. Will add xfail marks there also.

@github-actions crossbow submit hdfs

Revision: 481a85c

Submitted crossbow builds: ursacomputing/crossbow @ actions-fc72b69813

Task	Status
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1

Thanks a lot @AlenkaF for the work here!

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

There were 7 benchmark results indicating a performance regression:

• Commit Run on ursa-i9-9960x at 2023-12-22 00:21:42Z
  • file-read (R) with compression=uncompressed, dataset=fanniemae_2016Q4, file_type=feather, language=R, output_type=dataframe
  • wide-dataframe (Python) with use_legacy_dataset=true
• and 5 more (see the report linked below)

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

The wide-dataframe case seems a genuine perf regression (and not a flaky outlier as the other listed cases). That might mean that for wide dataframes, the new code path is slower compared to the legacy dataset reader (since with this commit, also when specifying use_legacy_dataset=True, the new code path will be used).
That seems to match with the timing in the use_legacy_dataset=False case of the wide-dataframe benchmark, as now both benchmarks more or less show the same timing.

However, I can't reproduce this locally with pyarrow 14.0 (where the legacy reader still exists):

import numpy as np
import pandas as pd
import pyarrow as pa
import pyarrow.parquet as pq

dataframe = pd.DataFrame(np.random.rand(100, 10000))
table = pa.Table.from_pandas(dataframe)
pq.write_table(table, "test_wide_dataframe.parquet")

In [7]: %timeit -r 50 pq.read_table("test_wide_dataframe.parquet", use_legacy_dataset=True)
392 ms ± 4.67 ms per loop (mean ± std. dev. of 50 runs, 1 loop each)

In [8]: %timeit -r 50 pq.read_table("test_wide_dataframe.parquet", use_legacy_dataset=False)
350 ms ± 11.5 ms per loop (mean ± std. dev. of 50 runs, 1 loop each)