This is an automated email from the ASF dual-hosted git repository.
felipecrv pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 0956f3f7c9 GH-40607: [C++] Rename `Function::is_impure()` to
`is_pure()` (#40608)
0956f3f7c9 is described below
commit 0956f3f7c9f8d4f976275cd670744b52ee30cbf3
Author: Felipe Oliveira Carvalho <[email protected]>
AuthorDate: Mon Mar 18 20:58:26 2024 -0300
GH-40607: [C++] Rename `Function::is_impure()` to `is_pure()` (#40608)
### Rationale for this change
Remove the property negation from the function name so that reasoning about
the boolean value is easier.
"is_impure = false" vs "is_pure = true"
"is_impure" vs "!is_pure"
### What changes are included in this PR?
- Renaming of the virtual function and overrides
- Renaming of constructor parameters and class attributes
- Swapping of the boolean constants
- Replace `is_impure()` calls with `!is_pure()`
- Undoing a documentation change done in #40396
### Are these changes tested?
By existing tests.
* GitHub Issue: #40607
Authored-by: Felipe Oliveira Carvalho <[email protected]>
Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
---
cpp/src/arrow/compute/expression.cc | 4 ++--
cpp/src/arrow/compute/expression_test.cc | 2 +-
cpp/src/arrow/compute/function.h | 19 ++++++++++---------
cpp/src/arrow/compute/kernels/scalar_random.cc | 2 +-
docs/source/cpp/compute.rst | 2 +-
5 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/cpp/src/arrow/compute/expression.cc
b/cpp/src/arrow/compute/expression.cc
index cc24429c8a..532869b345 100644
--- a/cpp/src/arrow/compute/expression.cc
+++ b/cpp/src/arrow/compute/expression.cc
@@ -845,7 +845,7 @@ Result<Expression> FoldConstants(Expression expr) {
std::move(expr), [](Expression expr) { return expr; },
[](Expression expr, ...) -> Result<Expression> {
auto call = CallNotNull(expr);
- if (call->function->is_impure()) return expr;
+ if (!call->function->is_pure()) return expr;
if (std::all_of(call->arguments.begin(), call->arguments.end(),
[](const Expression& argument) { return
argument.literal(); })) {
@@ -1085,7 +1085,7 @@ Result<Expression> Canonicalize(Expression expr,
compute::ExecContext* exec_cont
[&AlreadyCanonicalized, exec_context](Expression expr) ->
Result<Expression> {
auto call = expr.call();
if (!call) return expr;
- if (call->function->is_impure()) return expr;
+ if (!call->function->is_pure()) return expr;
if (AlreadyCanonicalized(expr)) return expr;
diff --git a/cpp/src/arrow/compute/expression_test.cc
b/cpp/src/arrow/compute/expression_test.cc
index 5c87736efb..30bd882b2c 100644
--- a/cpp/src/arrow/compute/expression_test.cc
+++ b/cpp/src/arrow/compute/expression_test.cc
@@ -1401,7 +1401,7 @@ TEST(Expression, SingleComparisonGuarantees) {
static Status RegisterMyRandom() {
const std::string name = "my_random";
auto func = std::make_shared<ScalarFunction>(name, Arity::Unary(),
FunctionDoc::Empty(),
- nullptr, /*is_impure=*/true);
+ nullptr, /*is_pure=*/false);
auto func_exec = [](KernelContext* /*ctx*/, const ExecSpan& /*batch*/,
ExecResult* /*out*/) -> Status { return Status::OK(); };
diff --git a/cpp/src/arrow/compute/function.h b/cpp/src/arrow/compute/function.h
index 23ff4dbcea..2b86f64216 100644
--- a/cpp/src/arrow/compute/function.h
+++ b/cpp/src/arrow/compute/function.h
@@ -231,9 +231,11 @@ class ARROW_EXPORT Function {
/// \brief Returns the pure property for this function.
///
- /// For impure functions like 'random', we should skip any simplification
- /// for this function except it's arguments.
- virtual bool is_impure() const { return false; }
+ /// Impure functions are those that may return different results for the same
+ /// input arguments. For example, a function that returns a random number is
+ /// not pure. An expression containing only pure functions can be simplified
by
+ /// pre-evaluating any sub-expressions that have constant arguments.
+ virtual bool is_pure() const { return true; }
protected:
Function(std::string name, Function::Kind kind, const Arity& arity,
FunctionDoc doc,
@@ -297,10 +299,10 @@ class ARROW_EXPORT ScalarFunction : public
detail::FunctionImpl<ScalarKernel> {
using KernelType = ScalarKernel;
ScalarFunction(std::string name, const Arity& arity, FunctionDoc doc,
- const FunctionOptions* default_options = NULLPTR, bool
is_impure = false)
+ const FunctionOptions* default_options = NULLPTR, bool
is_pure = true)
: detail::FunctionImpl<ScalarKernel>(std::move(name), Function::SCALAR,
arity,
std::move(doc), default_options),
- is_impure_(is_impure) {}
+ is_pure_(is_pure) {}
/// \brief Add a kernel with given input/output types, no required state
/// initialization, preallocation for fixed-width types, and default null
@@ -312,12 +314,11 @@ class ARROW_EXPORT ScalarFunction : public
detail::FunctionImpl<ScalarKernel> {
/// kernel's signature does not match the function's arity.
Status AddKernel(ScalarKernel kernel);
- /// \brief Impure property for expression simplification only takes
- /// effect in ScalarFunction.
- bool is_impure() const override { return is_impure_; }
+ /// \brief Returns the pure property for this function.
+ bool is_pure() const override { return is_pure_; }
private:
- bool is_impure_;
+ const bool is_pure_;
};
/// \brief A function that executes general array operations that may yield
diff --git a/cpp/src/arrow/compute/kernels/scalar_random.cc
b/cpp/src/arrow/compute/kernels/scalar_random.cc
index a3d3ec364e..517cf06867 100644
--- a/cpp/src/arrow/compute/kernels/scalar_random.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_random.cc
@@ -88,7 +88,7 @@ const FunctionDoc random_doc{
void RegisterScalarRandom(FunctionRegistry* registry) {
static auto random_options = RandomOptions::Defaults();
auto random_func = std::make_shared<ScalarFunction>(
- "random", Arity::Nullary(), random_doc, &random_options,
/*is_impure=*/true);
+ "random", Arity::Nullary(), random_doc, &random_options,
/*is_pure=*/false);
ScalarKernel kernel{{}, float64(), ExecRandom, RandomState::Init};
kernel.null_handling = NullHandling::OUTPUT_NOT_NULL;
DCHECK_OK(random_func->AddKernel(kernel));
diff --git a/docs/source/cpp/compute.rst b/docs/source/cpp/compute.rst
index e1c030b179..e7310d2c0c 100644
--- a/docs/source/cpp/compute.rst
+++ b/docs/source/cpp/compute.rst
@@ -343,7 +343,7 @@ equivalents above and reflects how they are implemented
internally.
+-------------------------+---------+------------------------------------+------------------------+----------------------------------+-----------+
| hash_count | Unary | Any |
Int64 | :struct:`CountOptions` | \(2) |
+-------------------------+---------+------------------------------------+------------------------+----------------------------------+-----------+
-| hash_count_all | Unary | |
Int64 | | |
+| hash_count_all | Nullary | |
Int64 | | |
+-------------------------+---------+------------------------------------+------------------------+----------------------------------+-----------+
| hash_count_distinct | Unary | Any |
Int64 | :struct:`CountOptions` | \(2) |
+-------------------------+---------+------------------------------------+------------------------+----------------------------------+-----------+
