Include writer code and writerVersion in ORC files #14458

vuule · 2023-11-20T23:06:46Z

Description

Closes #14325
Changes some of the metadata written to ORC file:

Include the (cuDF) writer code (5).
include writerVersion, with the value of 7; This value means that bugs up to ORC-517 are fixed. This version (6+ required) allows us to write the nanosecond statistics.

This change can have unexpected impact, depending on how readers use these fields.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

…fea-write_orc-version-and-code

davidwendt · 2023-11-22T22:10:17Z

cpp/src/io/orc/orc.hpp

@@ -40,6 +40,9 @@ namespace orc {
 static constexpr uint32_t block_header_size = 3;
 // Seconds from January 1st, 1970 to January 1st, 2015
 static constexpr int64_t orc_utc_epoch = 1420070400;
+// Each ORC writer implementation should write its code to the file footer
+// the codes are specified in the ORC specification
+static constexpr int32_t cudf_writer_code = 5;


We are ORC writer number 5?
Is there a link to these codes?

Yeah, we got assigned 5. Search for cudf in https://orc.apache.org/specification/ORCv1/

I can add the link above.

harrism · 2023-11-23T01:34:24Z

I think someone more familiar with ORC should review. Removing my assignment.

revans2

This looks good to me.

hyperbolic2346 · 2023-12-01T19:28:52Z

cpp/src/io/orc/writer_impl.cu

@@ -2702,7 +2703,8 @@ void writer::impl::close()
  ps.footerLength         = pbw.size();
  ps.compression          = _compression_kind;
  ps.compressionBlockSize = _compression_blocksize;
-  ps.version              = {0, 12};
+  ps.version              = {0, 12};  // Hive 0.12
+  ps.writerVersion        = 7;        // ORC-517 fixed


should this 7 be defined above like MAGIC with a comment about it? Do we change the writer version every major release or major changes? Is this something we never change?

My understanding is that we would increase this version when we implement newer features and fix notable bugs, see https://github.com/apache/orc/blob/main/proto/orc_proto.proto#L412-L422

I'm not sure if this should be a constant. I'll make the change if you think that's better.

If it is a constant it is easier to add a multi-line comment explaining that major changes should increment it. I'd also be ok with a comment about the version. At the extreme, I would have a changelist in the comments

// version 1: Initial implementation // version 2: added nested types // version 3: fixed bug xyz

Included the list of versions, let me know what you think

…fea-write_orc-version-and-code

ttnghia · 2023-12-06T23:21:44Z

cpp/src/io/orc/orc.hpp

+static constexpr int32_t cudf_writer_code = 5;
+// Each ORC writer implementation should write its version to the PostScript
+// The version values are based on the ORC Java writer bug fixes and features
+// From https://github.com/apache/orc/blob/main/proto/orc_proto.proto:


The link cannot be accessed.

yeah, they removed the proto module in a recent PR :(
I'll track down where the information is now.

fixed the link
what timing!

ttnghia · 2023-12-06T23:22:54Z

cpp/src/io/orc/orc.hpp

+  uint32_t compressionBlockSize{};        // the maximum size of each compression chunk
+  std::vector<uint32_t> version;          // the version of the file format [major, minor]
+  uint64_t metadataLength = 0;            // the length of the metadata section in bytes
+  std::optional<uint32_t> writerVersion;  // The version of the writer that wrote the file


Why this is an optional? Should it be a permanent field?

it's based on the protobuf specs: https://github.com/apache/orc/blob/branch-1.9/proto/orc_proto.proto#L439
We might always write the version now, but we should be able to read files without this field.

ttnghia · 2023-12-06T23:23:05Z

cpp/src/io/orc/orc.hpp

@@ -90,6 +113,7 @@ struct FileFooter {
  uint64_t numberOfRows = 0;               // the total number of rows in the file
  std::vector<ColStatsBlob> statistics;    // Column statistics blobs
  uint32_t rowIndexStride = 0;             // the maximum number of rows in each index entry
+  std::optional<uint32_t> writer;          // Writer code


Similarly, should it be a permanent field?

…fea-write_orc-version-and-code

vuule · 2023-12-07T17:48:15Z

/merge

Closes rapidsai#14325 Changes some of the metadata written to ORC file: - Include the (cuDF) writer code (5). - include writerVersion, with the value of 7; This value means that bugs up to ORC-517 are fixed. This version (6+ required) allows us to write the nanosecond statistics. This change can have unexpected impact, depending on how readers use these fields. Authors: - Vukasin Milovanovic (https://github.com/vuule) Approvers: - Robert (Bobby) Evans (https://github.com/revans2) - Nghia Truong (https://github.com/ttnghia) - Mike Wilson (https://github.com/hyperbolic2346) URL: rapidsai#14458

vuule added 2 commits November 20, 2023 13:36

read and write writer code

48ef562

correct version

c169b93

vuule added feature request New feature or request cuIO cuIO issue breaking Breaking change labels Nov 20, 2023

vuule self-assigned this Nov 20, 2023

github-actions bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. CMake CMake build issue conda Java Affects Java cuDF API. labels Nov 20, 2023

vuule changed the base branch from branch-23.12 to branch-24.02 November 20, 2023 23:07

enable nanoseconds

2d01dde

github-actions bot removed Python Affects Python cuDF API. CMake CMake build issue conda Java Affects Java cuDF API. labels Nov 20, 2023

vuule mentioned this pull request Nov 20, 2023

[BUG] ORC writer produce wrong timestamp metrics which causes spark not to do predicate push down #14325

Closed

vuule added 8 commits November 20, 2023 18:24

Merge branch 'branch-24.02' into fea-write_orc-version-and-code

bcde64f

go back to 6

e92d7f1

revert to 0.12; add writerVersion

f8e47f7

remove debug code

71d0188

Merge branch 'branch-24.02' of https://github.com/rapidsai/cudf into …

2397baf

…fea-write_orc-version-and-code

use s.writer instead of the constant directly

ae6bac7

expand comment

747d193

update tests

38b49ce

vuule changed the title ~~Include writer code and correct version in ORC files~~ Include writer code and writerVersion in ORC files Nov 21, 2023

thirtiseven mentioned this pull request Nov 22, 2023

Enable ORC timestamp and decimal predicate push down tests NVIDIA/spark-rapids#9827

Merged

vuule marked this pull request as ready for review November 22, 2023 02:40

vuule requested a review from a team as a code owner November 22, 2023 02:40

vuule requested review from harrism and davidwendt November 22, 2023 02:40

Merge branch 'branch-24.02' of https://github.com/rapidsai/cudf into …

de184e6

…fea-write_orc-version-and-code

davidwendt reviewed Nov 22, 2023

View reviewed changes

harrism removed their request for review November 23, 2023 01:34

vuule requested a review from davidwendt November 29, 2023 00:12

revans2 approved these changes Nov 29, 2023

View reviewed changes

hyperbolic2346 reviewed Dec 1, 2023

View reviewed changes

vuule added 2 commits December 5, 2023 13:14

Merge branch 'branch-24.02' of https://github.com/rapidsai/cudf into …

a7a7b9e

…fea-write_orc-version-and-code

write version detailed docs

5d69f6b

vuule requested a review from hyperbolic2346 December 5, 2023 22:03

ttnghia reviewed Dec 6, 2023

View reviewed changes

vuule added 2 commits December 6, 2023 15:49

Merge branch 'branch-24.02' of https://github.com/rapidsai/cudf into …

0552dd1

…fea-write_orc-version-and-code

fix link

7b49b8e

vuule requested a review from ttnghia December 6, 2023 23:54

ttnghia approved these changes Dec 7, 2023

View reviewed changes

hyperbolic2346 approved these changes Dec 7, 2023

View reviewed changes

rapids-bot bot merged commit b02e82f into rapidsai:branch-24.02 Dec 7, 2023
67 checks passed

vuule deleted the fea-write_orc-version-and-code branch December 7, 2023 17:48

vuule mentioned this pull request Dec 20, 2023

[FEA] Set the officially assigned id for the ORC writer implemented by cuDF #13977

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include writer code and writerVersion in ORC files #14458

Include writer code and writerVersion in ORC files #14458

vuule commented Nov 20, 2023 •

edited

Loading

davidwendt Nov 22, 2023

vuule Nov 27, 2023

harrism commented Nov 23, 2023

revans2 left a comment

hyperbolic2346 Dec 1, 2023

vuule Dec 4, 2023

hyperbolic2346 Dec 5, 2023

vuule Dec 5, 2023

ttnghia Dec 6, 2023

vuule Dec 6, 2023

vuule Dec 6, 2023

ttnghia Dec 6, 2023

vuule Dec 6, 2023

ttnghia Dec 6, 2023 •

edited

Loading

vuule commented Dec 7, 2023

Include writer code and writerVersion in ORC files #14458

Include writer code and writerVersion in ORC files #14458

Conversation

vuule commented Nov 20, 2023 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrism commented Nov 23, 2023

revans2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttnghia Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

vuule commented Dec 7, 2023

vuule commented Nov 20, 2023 •

edited

Loading

ttnghia Dec 6, 2023 •

edited

Loading