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

Jump to bottom

Fix default values for stream parameter #1583

Merged
merged 6 commits into from
Nov 21, 2023
Merged

Fix default values for stream parameter #1583

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d50f895
Fix stream
ttnghia Nov 21, 2023
135f05b
Merge branch 'branch-23.12' into test_row_conversion
ttnghia Nov 21, 2023
cfc4165
Remove empty comment lines
ttnghia Nov 21, 2023
1bf25eb
Add `#pragma once`
ttnghia Nov 21, 2023
9b6b528
Reformat
ttnghia Nov 21, 2023
3a3e0d9
Revert "Reformat"
ttnghia Nov 21, 2023
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
68 changes: 31 additions & 37 deletions src/main/cpp/src/decimal_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2022, NVIDIA CORPORATION.
* Copyright (c) 2022-2023, 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 All @@ -16,45 +16,39 @@

#include <cudf/column/column_view.hpp>
#include <cudf/table/table.hpp>
#include <cudf/utilities/default_stream.hpp>

//
Copy link
Member

Choose a reason for hiding this comment

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

Comment with no text? Should either remove these or add a comment. I'm assuming this is to delineate sections of the includes (IMO not necessary, pretty obvious from include paths or lack thereof), but without text it's not clear what the intent is.

Applies to everywhere this was added in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is due to the issue with clang-format that messes up the header order. I have to use an empty comment line to prevent header reordering.

Copy link
Member

Choose a reason for hiding this comment

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

Does it need to be empty, or can the comment say "// this is here to avoid clang-format from rearranging the header files". Also wondering why we're explicitly avoiding the recommended header order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The current clang-format config combines headers together and doesn't respect the conventional "near to far" order. That means we should include our header first, then library headers, then builtin headers. By using "near to far" order, we can ensure that most headers have their own dependencies included themselves.

We probably should change the config a little bit to account for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally if we switch to use .clang-format style from cudf then we don't have to worry about this as cudf config can avoid this mixing header issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is another option: fix the style to preserve header grouping: #1584. With that, the header organization is totally up to the code author.

#include <rmm/cuda_stream_view.hpp>

//
#include <cstddef>

namespace cudf::jni {

std::unique_ptr<cudf::table> multiply_decimal128(
cudf::column_view const& a,
cudf::column_view const& b,
int32_t product_scale,
rmm::cuda_stream_view stream = rmm::cuda_stream_default);

std::unique_ptr<cudf::table> divide_decimal128(
cudf::column_view const& a,
cudf::column_view const& b,
int32_t quotient_scale,
rmm::cuda_stream_view stream = rmm::cuda_stream_default);

std::unique_ptr<cudf::table> integer_divide_decimal128(
cudf::column_view const& a,
cudf::column_view const& b,
int32_t quotient_scale,
rmm::cuda_stream_view stream = rmm::cuda_stream_default);

std::unique_ptr<cudf::table> remainder_decimal128(
cudf::column_view const& a,
cudf::column_view const& b,
int32_t remainder_scale,
rmm::cuda_stream_view stream = rmm::cuda_stream_default);

std::unique_ptr<cudf::table> add_decimal128(
cudf::column_view const& a,
cudf::column_view const& b,
int32_t quotient_scale,
rmm::cuda_stream_view stream = rmm::cuda_stream_default);

std::unique_ptr<cudf::table> sub_decimal128(
cudf::column_view const& a,
cudf::column_view const& b,
int32_t quotient_scale,
rmm::cuda_stream_view stream = rmm::cuda_stream_default);
} // namespace cudf::jni
std::unique_ptr<cudf::table>
multiply_decimal128(cudf::column_view const &a, cudf::column_view const &b, int32_t product_scale,
rmm::cuda_stream_view stream = cudf::get_default_stream());

std::unique_ptr<cudf::table>
divide_decimal128(cudf::column_view const &a, cudf::column_view const &b, int32_t quotient_scale,
rmm::cuda_stream_view stream = cudf::get_default_stream());

std::unique_ptr<cudf::table>
integer_divide_decimal128(cudf::column_view const &a, cudf::column_view const &b,
int32_t quotient_scale,
rmm::cuda_stream_view stream = cudf::get_default_stream());

std::unique_ptr<cudf::table>
remainder_decimal128(cudf::column_view const &a, cudf::column_view const &b,
int32_t remainder_scale,
rmm::cuda_stream_view stream = cudf::get_default_stream());

std::unique_ptr<cudf::table>
add_decimal128(cudf::column_view const &a, cudf::column_view const &b, int32_t quotient_scale,
rmm::cuda_stream_view stream = cudf::get_default_stream());

std::unique_ptr<cudf::table>
sub_decimal128(cudf::column_view const &a, cudf::column_view const &b, int32_t quotient_scale,
rmm::cuda_stream_view stream = cudf::get_default_stream());
} // namespace cudf::jni
13 changes: 8 additions & 5 deletions src/main/cpp/src/parse_uri.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@

#include <cudf/strings/strings_column_view.hpp>
#include <cudf/types.hpp>
#include <cudf/utilities/default_stream.hpp>

//
#include <rmm/cuda_stream_view.hpp>

//
#include <memory>

namespace spark_rapids_jni {
Expand All @@ -33,9 +36,9 @@ namespace spark_rapids_jni {
* @param mr Memory resource for returned column
* @return std::unique_ptr<column> String column of protocols parsed.
*/
std::unique_ptr<cudf::column> parse_uri_to_protocol(
cudf::strings_column_view const& input,
rmm::cuda_stream_view stream = rmm::cuda_stream_default,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());
std::unique_ptr<cudf::column>
parse_uri_to_protocol(cudf::strings_column_view const &input,
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource *mr = rmm::mr::get_current_device_resource());

} // namespace spark_rapids_jni
} // namespace spark_rapids_jni
51 changes: 27 additions & 24 deletions src/main/cpp/src/row_conversion.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2022, NVIDIA CORPORATION.
* Copyright (c) 2022-2023, 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 All @@ -16,36 +16,39 @@

#pragma once

#include <memory>

//
#include <cudf/lists/lists_column_view.hpp>
#include <cudf/table/table_view.hpp>
#include <cudf/utilities/default_stream.hpp>

//
#include <rmm/cuda_stream_view.hpp>

//
#include <memory>

namespace spark_rapids_jni {

std::vector<std::unique_ptr<cudf::column>> convert_to_rows_fixed_width_optimized(
cudf::table_view const& tbl,
// TODO need something for validity
rmm::cuda_stream_view stream = rmm::cuda_stream_default,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());
cudf::table_view const &tbl,
// TODO need something for validity
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource *mr = rmm::mr::get_current_device_resource());

std::vector<std::unique_ptr<cudf::column>> convert_to_rows(
cudf::table_view const& tbl,
// TODO need something for validity
rmm::cuda_stream_view stream = rmm::cuda_stream_default,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());
std::vector<std::unique_ptr<cudf::column>>
convert_to_rows(cudf::table_view const &tbl,
// TODO need something for validity
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource *mr = rmm::mr::get_current_device_resource());

std::unique_ptr<cudf::table> convert_from_rows_fixed_width_optimized(
cudf::lists_column_view const& input,
std::vector<cudf::data_type> const& schema,
rmm::cuda_stream_view stream = rmm::cuda_stream_default,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

std::unique_ptr<cudf::table> convert_from_rows(
cudf::lists_column_view const& input,
std::vector<cudf::data_type> const& schema,
rmm::cuda_stream_view stream = rmm::cuda_stream_default,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

} // namespace spark_rapids_jni
cudf::lists_column_view const &input, std::vector<cudf::data_type> const &schema,
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource *mr = rmm::mr::get_current_device_resource());

std::unique_ptr<cudf::table>
convert_from_rows(cudf::lists_column_view const &input, std::vector<cudf::data_type> const &schema,
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource *mr = rmm::mr::get_current_device_resource());

} // namespace spark_rapids_jni