ARROW-5889: [C++][Parquet] Add property to indicate origin from converted type to TimestampLogicalType #4856

tpboudreau · 2019-07-11T16:23:16Z

This patch:

adds a new boolean member isFromConvertedType() to the TimestampLogicalType class that is set to "true" if the LogicalType was created from a converted type of TIMESTAMP_MILLIS or TIMESTAMP_MICROS
prevents writing the TimestampLogicalType in the Parquet schema if this new property is true
changes the Arrow reader to ignore the isAdjustedToUTC() property of the TimestampLogicalType if the type annotation came from a converted type

jorisvandenbossche

This looks good to me. Also tested it locally and can confirm this now properly reads older files in pyarrow.

tpboudreau · 2019-07-11T20:22:18Z

Thanks @jorisvandenbossche for testing it out. When you say it works on older files, did you use the files attached to the Jira issue by the reporter? I've been considering adding a test using those as fixtures. Thanks again.

jorisvandenbossche · 2019-07-11T20:37:45Z

Yes, I checked with those files (as well as something I wrote myself with an older version). I certainly think we should add such tests reading older files (I just opened https://issues.apache.org/jira/browse/ARROW-5915 for that).
But, if we do it like that, we should probably write a script that generates such files (so that it is reproducible) instead of adding the files attached to the issue. And also, we should discuss how to do a more fully integrated forward+backwards compatibility testing (although IMO that does not prevent adding some basic back compat tests)

tpboudreau · 2019-07-12T16:57:36Z

Great. Thanks for testing the reporter's files.

Thanks also for opening that issue for compatibility testing -- I generally agree with you -- I'll comment further on that Jira issue when I get the chance.

wesm · 2019-07-12T17:49:22Z

@tpboudreau this needs to be rebased now, let me know if you need help

tpboudreau · 2019-07-12T23:01:31Z

Rebased and RFR (I'll monitor the TravisCI build after it kicks off).

codecov-io · 2019-07-13T03:56:05Z

Codecov Report

Merging #4856 into master will decrease coverage by 11.39%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #4856       +/-   ##
===========================================
- Coverage   87.08%   75.68%    -11.4%     
===========================================
  Files         883       58      -825     
  Lines      116941     3735   -113206     
  Branches     1418        0     -1418     
===========================================
- Hits       101837     2827    -99010     
+ Misses      14742      908    -13834     
+ Partials      362        0      -362

Impacted Files	Coverage Δ
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
go/arrow/memory/memory_avx2_amd64.go
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/parquet/column_scanner.cc
...p/src/gandiva/precompiled/epoch_time_point_test.cc
cpp/src/parquet/thrift.h
... and 810 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e70357...7b51bba. Read the comment docs.

wesm · 2019-07-13T17:10:49Z

cpp/src/parquet/types.h

                                                 bool force_set_converted_type = false);
  bool is_adjusted_to_utc() const;
  LogicalType::TimeUnit::unit time_unit() const;

+  /// \brief If true, will not set LogicalType in Thrift metadata


This isn't true anymore -- I'll remove this comment and the is_serialized method

Sorry, GitHub was hiding files from me so this is indeed what is going on in this patch. Will take a closer look

wesm

+1. I see that the is_serialized override just allows us to round-trip the metadata faithfully. If a file is read to Arrow, then written back, then the logical type will be written (I think)

…rted type to TimestampLogicalType This patch: - adds a new boolean member isFromConvertedType() to the TimestampLogicalType class that is set to "true" if the LogicalType was created from a converted type of TIMESTAMP_MILLIS or TIMESTAMP_MICROS - prevents writing the TimestampLogicalType in the Parquet schema if this new property is true - changes the Arrow reader to ignore the isAdjustedToUTC() property of the TimestampLogicalType if the type annotation came from a converted type Author: TP Boudreau <tpboudreau@gmail.com> Closes #4856 from tpboudreau/ARROW-5889 and squashes the following commits: 458f063 <TP Boudreau> Add property showing converted type origin to TimestampLogicalType

…rted type to TimestampLogicalType This patch: - adds a new boolean member isFromConvertedType() to the TimestampLogicalType class that is set to "true" if the LogicalType was created from a converted type of TIMESTAMP_MILLIS or TIMESTAMP_MICROS - prevents writing the TimestampLogicalType in the Parquet schema if this new property is true - changes the Arrow reader to ignore the isAdjustedToUTC() property of the TimestampLogicalType if the type annotation came from a converted type Author: TP Boudreau <tpboudreau@gmail.com> Closes apache#4856 from tpboudreau/ARROW-5889 and squashes the following commits: 458f063 <TP Boudreau> Add property showing converted type origin to TimestampLogicalType

tpboudreau mentioned this pull request Jul 11, 2019

ARROW-5878: [C++][Parquet] Restore pre-0.14.0 Parquet forward compatibility by adding option to unconditionally set TIMESTAMP_MICROS/TIMESTAMP_MILLIS ConvertedType #4825

Closed

jorisvandenbossche reviewed Jul 11, 2019

View reviewed changes

wesm reviewed Jul 13, 2019

View reviewed changes

wesm approved these changes Jul 13, 2019

View reviewed changes

Add property showing converted type origin to TimestampLogicalType

458f063

wesm closed this in 99b9702 Jul 13, 2019

asfimport mentioned this pull request Aug 1, 2019

[Python][C++] Parquet backwards compat for timestamps without timezone broken #22303

Closed

jorisvandenbossche mentioned this pull request Jan 7, 2024

[C++][Parquet] Timestamp conversion from Parquet to Arrow does not follow compatibility guidelines for convertedType #39489

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-5889: [C++][Parquet] Add property to indicate origin from converted type to TimestampLogicalType #4856

ARROW-5889: [C++][Parquet] Add property to indicate origin from converted type to TimestampLogicalType #4856

tpboudreau commented Jul 11, 2019

jorisvandenbossche left a comment

tpboudreau commented Jul 11, 2019

jorisvandenbossche commented Jul 11, 2019

tpboudreau commented Jul 12, 2019

wesm commented Jul 12, 2019

tpboudreau commented Jul 12, 2019

codecov-io commented Jul 13, 2019

wesm Jul 13, 2019

wesm Jul 13, 2019

wesm left a comment

ARROW-5889: [C++][Parquet] Add property to indicate origin from converted type to TimestampLogicalType #4856

ARROW-5889: [C++][Parquet] Add property to indicate origin from converted type to TimestampLogicalType #4856

Conversation

tpboudreau commented Jul 11, 2019

jorisvandenbossche left a comment

Choose a reason for hiding this comment

tpboudreau commented Jul 11, 2019

jorisvandenbossche commented Jul 11, 2019

tpboudreau commented Jul 12, 2019

wesm commented Jul 12, 2019

tpboudreau commented Jul 12, 2019

codecov-io commented Jul 13, 2019

Codecov Report

wesm Jul 13, 2019

Choose a reason for hiding this comment

wesm Jul 13, 2019

Choose a reason for hiding this comment

wesm left a comment

Choose a reason for hiding this comment