Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Jan 10, 2024
+13 −4
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
98c17d0
write cudf version in parquet created_by metadata field
etseidl Jan 8, 2024
b6dae25
lowercase cu
etseidl Jan 8, 2024
3be8bc1
use lower case to match other parquet implementations
etseidl Jan 8, 2024
bdc597a
Merge branch 'branch-24.02' into writer_version
etseidl Jan 8, 2024
8c34446
Merge remote-tracking branch 'origin/branch-24.02' into writer_version
etseidl Jan 9, 2024
f8a2024
Merge branch 'branch-24.02' into writer_version
etseidl Jan 9, 2024
6e4614a
Merge branch 'writer_version' of github.com:etseidl/cudf into writer_…
etseidl Jan 9, 2024
8c9fcde
Merge remote-tracking branch 'origin/branch-24.02' into writer_version
etseidl Jan 9, 2024
275adbc
address review comments
etseidl Jan 9, 2024
79a2aba
remove created_by member from aggregate_metadata
etseidl Jan 9, 2024
c7a4ef4
make sure CUDF_VERSION is defined
etseidl Jan 9, 2024
16caa90
move check
etseidl Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# =============================================================================
# Copyright (c) 2018-2023, NVIDIA CORPORATION.
# Copyright (c) 2018-2024, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
# in compliance with the License. You may obtain a copy of the License at
Expand Down Expand Up @@ -658,6 +658,12 @@ set_source_files_properties(
PROPERTIES COMPILE_DEFINITIONS "_FILE_OFFSET_BITS=64"
)

set_property(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SOURCE src/io/parquet/writer_impl.cu
APPEND
PROPERTY COMPILE_DEFINITIONS "CUDF_VERSION=${PROJECT_VERSION}"
)

set_target_properties(
cudf
PROPERTIES BUILD_RPATH "\$ORIGIN"
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/io/parquet/writer_impl.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019-2023, NVIDIA CORPORATION.
* Copyright (c) 2019-2024, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do other libraries write here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@etseidl etseidl Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with punting on the hash

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

thrust::optional<std::vector<ColumnOrder>> column_orders = thrust::nullopt;
};

Expand Down
Loading