Skip to content

GH-46299: [C++][Compute] Don't use static inline const for default options #46303

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

Merged
kou merged 1 commit into from
May 9, 2025
Merged

GH-46299: [C++][Compute] Don't use static inline const for default options #46303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 18 additions & 6 deletions cpp/src/arrow/compute/kernels/vector_rank.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,9 +381,13 @@ class RankMetaFunction : public RankMetaFunctionBase<RankMetaFunction> {
}

RankMetaFunction()
: RankMetaFunctionBase("rank", Arity::Unary(), rank_doc, &kDefaultOptions) {}
: RankMetaFunctionBase("rank", Arity::Unary(), rank_doc, GetDefaultOptions()) {}
Copy link
Member

@bkietz bkietz May 5, 2025

Choose a reason for hiding this comment

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

@pitrou well it's not one line, but...

Suggested change
: RankMetaFunctionBase("rank", Arity::Unary(), rank_doc, GetDefaultOptions()) {}
: RankMetaFunctionBase("rank", Arity::Unary(), rank_doc, [] {
static const auto kDefaultOptions = RankOptions::Defaults();
return &kDefaultOptions;
}()) {}

An IIFE can introduce a static lifetime object too

Copy link
Member

@pitrou pitrou May 5, 2025

Choose a reason for hiding this comment

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

IIFE?

Copy link
Member

@bkietz bkietz May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, Immediately Invoked Function Expression. I don't believe it's an official C++ concept

The expression is (<<nullary lambda for scope>>) (/* called with no args */)

Copy link
Member

@pitrou pitrou May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do you think it would work to do something like:

// in .h file
template <typename OptionsType>
const FunctionOptions* StaticOptionsInit() {
  static const auto kDefaultOptions = OptionsType::Defaults();
  return &kDefaultOptions;
};

// in .cc file
class RankMetaFunction : public RankMetaFunctionBase<RankMetaFunction> {
 public:
  RankMetaFunction()
      : RankMetaFunctionBase("rank", Arity::Unary(), rank_doc, StaticOptionsInit<RankOptions>()) {}

Copy link
Member

@bkietz bkietz May 5, 2025

Choose a reason for hiding this comment

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

I'm not sure. It seems to work on my machine but I really expected this to be an ODR violation. On my machine, the static local becomes a weak symbol so the linker ignores multiple definitions and just uses the first.

After reading through the provisions for multiple definitions, I tried using an inline variable and that seemed to work also:

template <typename OptionsType>
inline const auto kDefault = OptionsType::Defaults();

I'll keep thinking about this.

Copy link
Member

@bkietz bkietz May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, sorry; that wasn't intended as a serious proposal for this issue- it was just another instance of me being surprised about ODR. I think your StaticOptionsInit is fine; I can't convince myself looking at the standard but here's some precedent. Given that the issue was raised in LLVM and nobody exclaimed about ODR, my intuition is just incorrect here. (Those issues discuss the section group in which the static is emitted, which shouldn't bother us since we're not planning constexpr construction of FunctionOptions)

Also StaticOptionsInit is neat, and maybe we should extend it to be

template <typename T>
const T &Default() {
  static const T kDefault;
  return kDefault;
};

Then it might replace multiple instances of the same pattern in accessors which must return const std::shared_ptr<T>& but don't have one, as in FileSource::path

Copy link
Member

@pitrou pitrou May 9, 2025

Choose a reason for hiding this comment

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

Also StaticOptionsInit is neat, and maybe we should extend it to be

Given the LLVM issue you linked to above, I'm now convinced we shouldn't do something like this :)

Copy link
Member Author

@kou kou May 9, 2025

Choose a reason for hiding this comment

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

Finally, what approach should we use here? :-)

Copy link
Member

@pitrou pitrou May 9, 2025

Choose a reason for hiding this comment

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

Sorry @kou. I think we can use either GetDefaultOptions or the one suggested in #46303 (comment) . At your preference :)

Copy link
Member Author

@kou kou May 9, 2025

Choose a reason for hiding this comment

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

OK. Let's choose GetDefaultOptions() here. GetDefaultOptions() is more straightforward than #46303 (comment) because developers don't need to know IIFE and static variable lifetime.


static inline const auto kDefaultOptions = RankOptions::Defaults();
private:
static const RankOptions* GetDefaultOptions() {
static const auto kDefaultOptions = RankOptions::Defaults();
return &kDefaultOptions;
}
};

class RankQuantileMetaFunction : public RankMetaFunctionBase<RankQuantileMetaFunction> {
Expand All @@ -398,9 +402,13 @@ class RankQuantileMetaFunction : public RankMetaFunctionBase<RankQuantileMetaFun

RankQuantileMetaFunction()
: RankMetaFunctionBase("rank_quantile", Arity::Unary(), rank_quantile_doc,
&kDefaultOptions) {}
GetDefaultOptions()) {}

static inline const auto kDefaultOptions = RankQuantileOptions::Defaults();
private:
static const RankQuantileOptions* GetDefaultOptions() {
static const auto kDefaultOptions = RankQuantileOptions::Defaults();
return &kDefaultOptions;
}
};

class RankNormalMetaFunction : public RankMetaFunctionBase<RankNormalMetaFunction> {
Expand All @@ -415,9 +423,13 @@ class RankNormalMetaFunction : public RankMetaFunctionBase<RankNormalMetaFunctio

RankNormalMetaFunction()
: RankMetaFunctionBase("rank_normal", Arity::Unary(), rank_normal_doc,
&kDefaultOptions) {}
GetDefaultOptions()) {}

static inline const auto kDefaultOptions = RankQuantileOptions::Defaults();
private:
static const RankQuantileOptions* GetDefaultOptions() {
static const auto kDefaultOptions = RankQuantileOptions::Defaults();
return &kDefaultOptions;
}
};

} // namespace
Expand Down
19 changes: 19 additions & 0 deletions cpp/src/arrow/compute/kernels/vector_sort_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "arrow/array/concatenate.h"
#include "arrow/compute/api_vector.h"
#include "arrow/compute/kernels/test_util_internal.h"
#include "arrow/compute/registry.h"
#include "arrow/result.h"
#include "arrow/table.h"
#include "arrow/testing/gtest_util.h"
Expand Down Expand Up @@ -2317,6 +2318,11 @@ class TestRank : public BaseTestRank {
}
};

TEST_F(TestRank, DefaultOptions) {
ASSERT_OK_AND_ASSIGN(auto function, GetFunctionRegistry()->GetFunction("rank"));
ASSERT_STREQ(function->default_options()->type_name(), "RankOptions");
}

TEST_F(TestRank, Real) {
for (auto real_type : ::arrow::FloatingPointTypes()) {
SetInput(ArrayFromJSON(real_type, "[2.1, 3.2, 1.0, 0.0, 5.5]"));
Expand Down Expand Up @@ -2635,6 +2641,12 @@ class TestRankQuantile : public BaseTestRank {
}
};

TEST_F(TestRankQuantile, DefaultOptions) {
ASSERT_OK_AND_ASSIGN(auto function,
GetFunctionRegistry()->GetFunction("rank_quantile"));
ASSERT_STREQ(function->default_options()->type_name(), "RankQuantileOptions");
}

TEST_F(TestRankQuantile, Real) {
for (auto type : ::arrow::FloatingPointTypes()) {
AssertRankQuantileNumeric(type);
Expand Down Expand Up @@ -2675,5 +2687,12 @@ TEST_F(TestRankQuantile, FixedSizeBinary) {
AssertRankQuantile_N1N2N();
}

class TestRankNormal : public BaseTestRank {};

TEST_F(TestRankNormal, DefaultOptions) {
ASSERT_OK_AND_ASSIGN(auto function, GetFunctionRegistry()->GetFunction("rank_normal"));
ASSERT_STREQ(function->default_options()->type_name(), "RankQuantileOptions");
}

} // namespace compute
} // namespace arrow
Loading