ARROW-9390: [C++][Doc] Review compute function names by pitrou · Pull Request #7755 · apache/arrow

pitrou · 2020-07-14T13:54:27Z

Modified function names:

minmax -> min_max
binary_isascii -> string_isascii (only works on string types)
ascii_length -> binary_length (also make it work on binary types)
binary_contains_exact -> match_substring
(other possibility: has_substring ?)
match -> index_in
isin -> is_in
list_value_lengths -> list_value_length
partition_indices -> partition_nth_indices
(other kinds of partitioning would be possible, e.g. using a predicate)

Document string predicate functions (ARROW-9444).

Also fix the allocation of IsValid output buffer in certain cases.

pitrou · 2020-07-14T13:56:25Z

Also cc @xhochy @nealrichardson

pitrou · 2020-07-14T14:00:08Z

Valgrind works fine on this PR.

Modified function names: * minmax -> min_max * binary_isascii -> string_isascii (only works on string types) * ascii_length -> binary_length (also make it work on binary types) * binary_contains_exact -> match_substring (other possibility: has_substring ?) * match -> index_in * isin -> is_in * list_value_lengths -> list_value_length * partition_indices -> partition_nth_indices (other kinds of partitioning would be possible, e.g. using a predicate) Document string predicate functions (ARROW-9444). Also fix the allocation of IsValid output buffer in certain cases.

pitrou · 2020-07-14T14:18:37Z

Rebased and fixed CI (hopefully).

jorisvandenbossche · 2020-07-14T14:24:35Z

binary_contains_exact -> match_substring (other possibility: has_substring ?)

On the PR (#7593 (comment)), @xhochy mentioned that the "exact" part was important to him

pitrou · 2020-07-14T14:26:03Z

Well, "substring" clearly implies "exact". As for case-sensitivity, I hope that can be a flag in the options, because I don't think we want a combinatorial explosion of string matching functions.

jorisvandenbossche · 2020-07-14T14:27:23Z

match -> index_in

+1, that's a much clearer name IMO (although the R people might disagree ;-))

github-actions · 2020-07-14T14:35:40Z

https://issues.apache.org/jira/browse/ARROW-9390

wesm

Thanks for doing this. Some minor comments which can be addressed as follow up also

wesm · 2020-07-14T14:23:51Z

cpp/src/arrow/compute/kernels/scalar_string.cc

-  auto func = std::make_shared<ScalarFunction>("binary_contains_exact", Arity::Unary());
-  auto exec_32 = BinaryContainsExact<StringType>::Exec;
-  auto exec_64 = BinaryContainsExact<LargeStringType>::Exec;
+void AddMatchSubstring(FunctionRegistry* registry) {


I think this is fine, we can use match_substring_case_insensitive for the case insensitive version

cc @xhochy

Yes, this is fine with me.

cpp/src/arrow/compute/kernels/test_util.cc

cpp/src/arrow/compute/kernels/vector_nested_test.cc

cpp/src/arrow/compute/kernels/scalar_validity.cc

wesm · 2020-07-14T14:37:45Z

Does this conflict with 1d7d919 which I just merged?

pitrou · 2020-07-14T14:59:16Z

No, it's rebased already.

pitrou · 2020-07-14T16:01:17Z

AppVeyor build: https://ci.appveyor.com/project/pitrou/arrow/builds/34089592
Github Actions: https://github.com/pitrou/arrow/runs/869791845

pitrou · 2020-07-14T16:01:34Z

I think this can be merged. Any concerns?

wesm · 2020-07-14T16:02:55Z

Nope. +1 here

nealrichardson

Sorry I didn't get to this before this merged, but a couple of thoughts.

nealrichardson · 2020-07-14T17:01:24Z

cpp/src/arrow/compute/kernels/scalar_string.cc

  MakeUnaryStringBatchKernel<AsciiLower>("ascii_lower", registry);

-  AddUnaryStringPredicate<IsAscii>("binary_isascii", registry);
+  AddUnaryStringPredicate<IsAscii>("string_isascii", registry);


Why isn't this string_is_ascii? It seems we prefer is_x to isx (cf. isin -> is_in in this PR).

That's a good point. Should we make a followup PR?
(if it's me, it's only tomorrow)

I can make this change today unless someone else wants to

Also ascii_is_alnum, etc. ;-)

nealrichardson · 2020-07-14T17:03:13Z

cpp/src/arrow/compute/kernels/scalar_string_benchmark.cc

-  UnaryStringBenchmark(state, "binary_contains_exact", &options);
+static void MatchSubstring(benchmark::State& state) {
+  MatchSubstringOptions options("abac");
+  UnaryStringBenchmark(state, "match_substring", &options);


Are there clear criteria for when the function gets a binary_ or string_ prefix and when it doesn't?

Related, if we haven't documented the naming conventions (haven't seen it yet but I'll keep reading), we should so that future developers/selves know what to call new functions.

I don't think so. Personally, I find prefixes a bit pointless or even distracting.
(or, here, plain misleading, since "binary_contains_exact" didn't work on binary arrays, only string arrays...)

Perhaps someone else wants to think about a convention?

Followup from PR apache#7755: a new doc file wasn't checked in, and Sphinx only emits a warning.

Followup from PR #7755: a new doc file wasn't checked in, and Sphinx only emits a warning. Closes #7760 from pitrou/ARROW-9390-missing-file Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

pitrou requested a review from wesm July 14, 2020 13:54

pitrou force-pushed the ARROW-9390-compute-func-names branch from 0f1f905 to 66c0824 Compare July 14, 2020 14:18

Hopefully fix CI failure

8df2870

wesm reviewed Jul 14, 2020

View reviewed changes

Address review comments

762dc8e

pitrou closed this in 10289a0 Jul 14, 2020

pitrou deleted the ARROW-9390-compute-func-names branch July 14, 2020 16:05

nealrichardson reviewed Jul 14, 2020

View reviewed changes

pitrou added a commit to pitrou/arrow that referenced this pull request Jul 14, 2020

ARROW-9390: [Doc] Add missing file

59130d7

Followup from PR apache#7755: a new doc file wasn't checked in, and Sphinx only emits a warning.

pitrou mentioned this pull request Jul 14, 2020

ARROW-9390: [Doc] Add missing file #7760

Closed

asfimport mentioned this pull request Jul 14, 2020

[C++] Review compute function names #17265

Closed

Conversation

pitrou commented Jul 14, 2020

Uh oh!

pitrou commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Jul 14, 2020

Uh oh!

pitrou commented Jul 14, 2020

Uh oh!

jorisvandenbossche commented Jul 14, 2020

Uh oh!

pitrou commented Jul 14, 2020

Uh oh!

jorisvandenbossche commented Jul 14, 2020

Uh oh!

github-actions bot commented Jul 14, 2020

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wesm commented Jul 14, 2020

Uh oh!

pitrou commented Jul 14, 2020

Uh oh!

pitrou commented Jul 14, 2020

Uh oh!

pitrou commented Jul 14, 2020

Uh oh!

wesm commented Jul 14, 2020

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pitrou commented Jul 14, 2020 •

edited

Loading