GH-35746: [Parquet][C++][Python] Switch default Parquet version to 2.6 by anjakefala · Pull Request #36137 · apache/arrow

anjakefala · 2023-06-16T23:14:31Z

Change the default parquet version to 2.6.

Discussed in:

… to 2.6

github-actions · 2023-06-16T23:14:52Z

⚠️ GitHub issue #35746 has been automatically assigned in GitHub to PR creator.

jorisvandenbossche · 2023-06-27T14:52:31Z

Taking a quick look at some of the Python failures:

test_write_error_deletes_incomplete_file: I assume the test relied on parquet.write_table to raise an error because of nanosecods being present, and they would get truncated. So either we need to add something else that errors, or we can add version="2.4" to keep it using the older parquet version and thus it still errors
test_timestamp_restore_timezone_nanosecond: it is creating the expected result with microseconds, but now it comes back as nanoseconds. I think here we want to test explicitly that the timezone is preserved when the resolution changes, so I would also add a version="2.4" to keep the test as it was. But you could parametrize the test just above to use different resolutions (ms, us, ns) to cover all of them.
test_parquet_version_timestamp_differences: also needs to be updated because the expected now contains nanoseconds instead of microseconds (but there are some comments there that are a bit outdated it seems ..)

jorisvandenbossche · 2023-06-27T14:54:25Z

For C++, the test failures are in

 [  FAILED  ] 2 tests, listed below:
[  FAILED  ] TestArrowReadWrite.DateTimeTypes
[  FAILED  ] TestArrowReadWrite.ParquetVersionTimestampDifferences

and I suppose those will also be updated to reflect the new defaults (nanoseconds are preserved), or updated to test both old default of 2.4 and new of 2.6

anjakefala · 2023-06-27T18:27:57Z

@jorisvandenbossche This was so helpful!

Yeah, with the _check_roundtrip calls, it seems that when the version is not explicitly set, it uses the default for write_table. And sometimes, the assumption was that this default version was still '1.0'. Tested behaviour didn't change for 2.4 and so it didn't matter that the new default version was being used for _check_roundtrip.

jorisvandenbossche

The python changes all look good!

@mapleFU could you have a look at the C++ test changes?
At the moment it is mostly updating the tests for the new default, not fully sure how well the explicit versions are covered in other tests.

jorisvandenbossche · 2023-07-03T08:44:51Z

python/pyarrow/tests/parquet/test_datetime.py

+        _check_roundtrip(table, expected, version=ver, coerce_timestamps='us')

    # TODO: after pyarrow allows coerce_timestamps='ns', tests like the
    # following should pass ...


Could also be left for a follow-up, but we could now allow this (in _create_arrow_writer_properties in _parquet.pyx we would need to update the cython bindings to pass it through, I think on C++ side it's already implemented)

jorisvandenbossche · 2023-07-03T08:50:06Z

cpp/src/parquet/arrow/arrow_reader_writer_test.cc

    arrow_writer_props.store_schema();
    if (inner_type->id() == ::arrow::Type::UINT32) {
-      writer_props.version(ParquetVersion::PARQUET_2_4);
+      writer_props.version(ParquetVersion::PARQUET_2_6);


The default writer_props created above with WriterProperties::Builder(); will now use 2_6 by default, so potentially this could be removed alltogether

cpp/src/parquet/column_writer_test.cc

mapleFU · 2023-07-03T09:21:20Z

cpp/src/parquet/arrow/arrow_schema_test.cc

       -1},
      {"timestamp(nanosecond, CET)", ::arrow::timestamp(::arrow::TimeUnit::NANO, "CET"),
-       LogicalType::Timestamp(true, LogicalType::TimeUnit::MICROS), ParquetType::INT64,
+       LogicalType::Timestamp(true, LogicalType::TimeUnit::NANOS), ParquetType::INT64,


Should we extract NANOS, and test with both 2_4 and 2_6? Since 2.4 might still be used for some time

mapleFU · 2023-07-03T09:25:39Z

The test in C++ looks ok. But I'm afraid that we might leak some test under 2.4, so maybe we can separate and test both 2.4 and 2.6?

anjakefala · 2023-07-05T22:49:40Z

AsofJoinTest needs to be updated as well.

jorisvandenbossche · 2023-07-06T07:08:05Z

@github-actions crossbow submit -g integration

github-actions · 2023-07-06T07:10:56Z

Revision: 932de52

Submitted crossbow builds: ursacomputing/crossbow @ actions-34c5b677fb

Task	Status
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-master
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.1.2
test-conda-python-3.9-pandas-latest
test-conda-python-3.9-spark-v3.2.0

cpp/src/parquet/column_writer_test.cc

mapleFU · 2023-07-06T07:23:41Z

cpp/src/parquet/arrow/arrow_reader_writer_test.cc

                              field("ts:us", t_us), field("ts:ns", t_ns)});
  auto input_table = Table::Make(input_schema, {a_s, a_ms, a_us, a_ns});

-  auto parquet_version_1_properties = ::parquet::default_writer_properties();


nice catch!

Co-authored-by: mwish <1506118561@qq.com>

jorisvandenbossche · 2023-07-06T07:44:20Z

AsofJoinTest needs to be updated as well.

I think this failure is not related, the AsofJoinTest regularly fails on main as well. See #36482

jorisvandenbossche · 2023-07-06T13:10:19Z

There might still be a few cases where the test coverage for all options can be improved (see some of the comments), but if needed, let's do that in a follow-up PR.
I am going to merge this now, because we have another PR waiting on this one that we also want to get in before the feature freeze.

jorisvandenbossche · 2023-07-06T13:10:53Z

Thanks @anjakefala!

conbench-apache-arrow · 2023-07-12T13:57:52Z

Conbench analyzed the 6 benchmark runs on commit 22f7e597.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

apacheGH-35746: [Parquet][C++][Python] Switch default Parquet version…

14b5c6f

… to 2.6

anjakefala requested review from AlenkaF and wjones127 as code owners June 16, 2023 23:14

github-actions bot added Component: C++ Component: Parquet Component: Python awaiting review Awaiting review labels Jun 16, 2023

anjakefala marked this pull request as draft June 19, 2023 17:25

Merge branch 'main' into kef/35746

ed53e1c

tests: add 2.6 cases to test_parquet_version_timestamp_differences

53c0e47

anjakefala added 2 commits June 30, 2023 11:03

Merge branch 'main' into kef/35746

4095601

tests: update pyarrow tests for parquet 2.4 -> 2.6

6107218

danepitkin mentioned this pull request Jun 30, 2023

GH-33321: [Python] Support converting to non-nano datetime64 for pandas >= 2.0 #35656

Merged

jorisvandenbossche reviewed Jul 3, 2023

View reviewed changes

jorisvandenbossche added this to the 13.0.0 milestone Jul 3, 2023

github-actions bot added awaiting changes Awaiting changes awaiting review Awaiting review and removed awaiting review Awaiting review labels Jul 3, 2023

mapleFU reviewed Jul 3, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Jul 3, 2023

anjakefala added 3 commits July 5, 2023 14:30

Update DateTimeTypes test

213275b

Ensure parquet_version_1_0_properties are actually set to 1.0 properties

115030c

Ran linter

932de52

anjakefala marked this pull request as ready for review July 5, 2023 22:04

mapleFU approved these changes Jul 6, 2023

View reviewed changes

Update cpp/src/parquet/column_writer_test.cc

b86b420

Co-authored-by: mwish <1506118561@qq.com>

jorisvandenbossche approved these changes Jul 6, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 6, 2023

jorisvandenbossche merged commit 22f7e59 into apache:main Jul 6, 2023

jorisvandenbossche removed the awaiting merge Awaiting merge label Jul 6, 2023

jorisvandenbossche mentioned this pull request Jul 7, 2023

GH-36537: [Python] Ensure dataset writer follows default Parquet version of 2.6 #36538

Merged

kylebarron mentioned this pull request Aug 17, 2023

Update default version in parquet.rst #37227

Open

mapleFU mentioned this pull request Oct 3, 2023

[Python] pandas to_parquet() writes Timestamps in a method unreadable by Redshift Spectrum #38000

Closed

Conversation

anjakefala commented Jun 16, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 16, 2023

Uh oh!

jorisvandenbossche commented Jun 27, 2023

Uh oh!

jorisvandenbossche commented Jun 27, 2023

Uh oh!

anjakefala commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mapleFU Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anjakefala commented Jul 5, 2023

Uh oh!

jorisvandenbossche commented Jul 6, 2023

Uh oh!

github-actions bot commented Jul 6, 2023

Uh oh!

Uh oh!

mapleFU Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche commented Jul 6, 2023

Uh oh!

jorisvandenbossche commented Jul 6, 2023

Uh oh!

jorisvandenbossche commented Jul 6, 2023

Uh oh!

conbench-apache-arrow bot commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anjakefala commented Jun 16, 2023 •

edited by github-actions bot

Loading

anjakefala commented Jun 27, 2023 •

edited

Loading

mapleFU commented Jul 3, 2023 •

edited

Loading