Write cuDF version in Parquet "created_by" metadata field #14721

etseidl · 2024-01-08T19:20:01Z

Description

Populate the informational created_by field in the Parquet file metadata. Identifying the source of a parquet file can help with tracking down interoperability problems.

Checklist

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

copy-pr-bot · 2024-01-08T19:20:05Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

vuule · 2024-01-08T19:22:54Z

/ok to test

vuule · 2024-01-08T20:53:48Z

cpp/src/io/parquet/writer_impl.cu

@@ -171,7 +171,7 @@ struct aggregate_writer_metadata {
    std::vector<std::vector<uint8_t>> column_indexes;
  };
  std::vector<per_file_metadata> files;
-  std::string created_by                                   = "";
+  std::string created_by = "cuDF Version " CUDF_STRINGIFY(CUDF_VERSION);


what do other libraries write here?

stuff like "parquet-cpp-arrow version 12.0.1" or "parquet-mr version 1.11.1 (build 765bd5cd7fdef2af1cecd0755000694b992bfadd)"

Should we align with those libraries on using version (lowercase) instead of Version (uppercase)? What about cuDF vs. cudf vs. libcudf?

I'm good with any of those...just checked arrow-rs and they also use lower case "version", so I'll change that. I wasn't sure what to use for the name..."libcudf" was my first choice actually. I'll go with that if there are no objections.

Apparently the parquet spec actually specifies a format 😮

<Application> version <App Version> (build <App Build Hash>)

Is there a build hash available anywhere? I found a suggestion to use git log -1 --format=%H. I could add that to the properties for building writer_impl.cu.

Managed to get a build hash, but keeping that up to date is beyond my minimal CMake skills. Given that neither arrow-cpp nor arrow-rs feel the need to include the build number, perhaps it's best to punt on that for now.

I'm okay with punting on build hashes. How do you feel about that @vuule? If it's important we can inject some CMake define for CUDF_BUILD_HASH and use CUDF_STRINGIFY(CUDF_BUILD_HASH) or similar.

I'm fine with punting on the hash

cudf is my preference, after further consideration. Python libraries will also call this code, and there is only one "cudf" implementation. Also the other libraries aren't including a "lib" prefix for compiled libraries, so it might be more natural to call it cudf.

I'm okay with cuDF or cudf. FWIW, the ORC specs call us "CUDF".

How about cudF....kinda looks like a race car then 🤣

…version

cpp/src/io/parquet/writer_impl.cu

ttnghia · 2024-01-09T05:24:35Z

cpp/CMakeLists.txt

@@ -658,6 +658,12 @@ set_source_files_properties(
  PROPERTIES COMPILE_DEFINITIONS "_FILE_OFFSET_BITS=64"
 )

+set_property(


The line 650 above this is using set_source_files_properties so should we use it here?

I'm not a CMake expert 😅. I used set_property since it's also used below for setting the same property for jit/cache.cpp.

I'm also not cmake expert. Okay probably they both can achieve the same output 👍

set_property is probably easier to work with here because you can APPEND with it. This solution is fine.

cpp/src/io/parquet/writer_impl.cu

vuule · 2024-01-09T18:30:07Z

/ok to test

vuule · 2024-01-09T20:31:15Z

That's it, I'm not okaying to test until I see @ttnghia 's approval! :D

ttnghia · 2024-01-09T20:39:25Z

cpp/src/io/parquet/writer_impl.cu

+#ifndef CUDF_VERSION
+#error "CUDF_VERSION is not defined"
+#endif


This is in the middle of the aggregate_writer_metadata class so the indentation here is terrible. I would recommend moving it to the top of this file, or moving it inside of the get_metadata below, just right before line 115.

I wanted it close to where it's used, but it looked uglier to me in the middle of the function. Moved it to the top just after the includes.

ttnghia · 2024-01-09T20:52:19Z

/ok to test

ttnghia · 2024-01-09T20:52:33Z

That's it, I'm not okaying to test until I see @ttnghia 's approval! :D

Approved, finally 😄

vyasr · 2024-01-10T20:41:48Z

cpp/CMakeLists.txt

@@ -658,6 +658,12 @@ set_source_files_properties(
  PROPERTIES COMPILE_DEFINITIONS "_FILE_OFFSET_BITS=64"
 )

+set_property(


set_property is probably easier to work with here because you can APPEND with it. This solution is fine.

vuule · 2024-01-10T21:43:53Z

/merge

etseidl added 2 commits January 8, 2024 11:13

write cudf version in parquet created_by metadata field

98c17d0

lowercase cu

b6dae25

etseidl requested review from a team as code owners January 8, 2024 19:20

etseidl requested review from bdice and vuule January 8, 2024 19:20

github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Jan 8, 2024

vuule added feature request New feature or request cuIO cuIO issue non-breaking Non-breaking change labels Jan 8, 2024

vuule reviewed Jan 8, 2024

View reviewed changes

etseidl and others added 5 commits January 8, 2024 13:43

use lower case to match other parquet implementations

3be8bc1

Merge branch 'branch-24.02' into writer_version

bdc597a

Merge remote-tracking branch 'origin/branch-24.02' into writer_version

8c34446

Merge branch 'branch-24.02' into writer_version

f8a2024

Merge branch 'writer_version' of github.com:etseidl/cudf into writer_…

6e4614a

…version

ttnghia reviewed Jan 9, 2024

View reviewed changes

cpp/src/io/parquet/writer_impl.cu Outdated Show resolved Hide resolved

ttnghia reviewed Jan 9, 2024

View reviewed changes

etseidl added 3 commits January 9, 2024 09:23

Merge remote-tracking branch 'origin/branch-24.02' into writer_version

8c9fcde

address review comments

275adbc

remove created_by member from aggregate_metadata

79a2aba

ttnghia reviewed Jan 9, 2024

View reviewed changes

cpp/src/io/parquet/writer_impl.cu Show resolved Hide resolved

vuule approved these changes Jan 9, 2024

View reviewed changes

make sure CUDF_VERSION is defined

c7a4ef4

ttnghia reviewed Jan 9, 2024

View reviewed changes

move check

16caa90

ttnghia approved these changes Jan 9, 2024

View reviewed changes

vyasr approved these changes Jan 10, 2024

View reviewed changes

rapids-bot bot merged commit 1078326 into rapidsai:branch-24.02 Jan 10, 2024
67 checks passed

etseidl deleted the writer_version branch January 10, 2024 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write cuDF version in Parquet "created_by" metadata field #14721

Write cuDF version in Parquet "created_by" metadata field #14721

etseidl commented Jan 8, 2024 •

edited

Loading

copy-pr-bot bot commented Jan 8, 2024

vuule commented Jan 8, 2024

vuule Jan 8, 2024

etseidl Jan 8, 2024

bdice Jan 8, 2024

etseidl Jan 8, 2024

etseidl Jan 8, 2024 •

edited

Loading

bdice Jan 9, 2024

vuule Jan 9, 2024

bdice Jan 9, 2024

vuule Jan 9, 2024

etseidl Jan 9, 2024

ttnghia Jan 9, 2024

etseidl Jan 9, 2024

ttnghia Jan 9, 2024

vyasr Jan 10, 2024

vuule commented Jan 9, 2024

vuule commented Jan 9, 2024

ttnghia Jan 9, 2024 •

edited

Loading

etseidl Jan 9, 2024

ttnghia commented Jan 9, 2024

ttnghia commented Jan 9, 2024

vyasr Jan 10, 2024

vuule commented Jan 10, 2024

Write cuDF version in Parquet "created_by" metadata field #14721

Write cuDF version in Parquet "created_by" metadata field #14721

Conversation

etseidl commented Jan 8, 2024 • edited Loading

Description

Checklist

copy-pr-bot bot commented Jan 8, 2024

vuule commented Jan 8, 2024

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

etseidl Jan 8, 2024 • edited Loading

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

vuule commented Jan 9, 2024

vuule commented Jan 9, 2024

ttnghia Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttnghia commented Jan 9, 2024

ttnghia commented Jan 9, 2024

Choose a reason for hiding this comment

vuule commented Jan 10, 2024

etseidl commented Jan 8, 2024 •

edited

Loading

etseidl Jan 8, 2024 •

edited

Loading

ttnghia Jan 9, 2024 •

edited

Loading