Skip to content

ARROW-9390: [C++][Doc] Review compute function names - #7755

Closed
pitrou wants to merge 3 commits into
apache:masterfrom
pitrou:ARROW-9390-compute-func-names
Closed

ARROW-9390: [C++][Doc] Review compute function names#7755
pitrou wants to merge 3 commits into
apache:masterfrom
pitrou:ARROW-9390-compute-func-names

Conversation

@pitrou

@pitroupitrou commented Jul 14, 2020

Copy link
Copy Markdown
Member

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
pitrou requested a review from wesmJuly 14, 2020 13:54
@pitrou

pitrou commented Jul 14, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Also cc @xhochy@nealrichardson

@pitrou

pitrou commented Jul 14, 2020

Copy link
Copy Markdown
MemberAuthor

Valgrind works fine on this PR.

@pitrou
ARROW-9390: [C++][Doc] Review compute function names
66c0824
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
pitrouforce-pushed the ARROW-9390-compute-func-names branch from 0f1f905 to 66c0824CompareJuly 14, 2020 14:18
@pitrou

pitrou commented Jul 14, 2020

Copy link
Copy Markdown
MemberAuthor

Rebased and fixed CI (hopefully).

@jorisvandenbossche

jorisvandenbossche commented Jul 14, 2020

Copy link
Copy Markdown
Member

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

pitrou commented Jul 14, 2020

Copy link
Copy Markdown
MemberAuthor

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

jorisvandenbossche commented Jul 14, 2020

Copy link
Copy Markdown
Member

match -> index_in

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

@github-actions

github-actionsBot commented Jul 14, 2020

Copy link
Copy Markdown

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

wesm
wesm reviewed Jul 14, 2020
View reviewed changes

@wesmwesm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment threadcpp/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) {

@wesmwesmJul 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

cc @xhochy

@xhochyxhochyJul 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, this is fine with me.

Comment threadcpp/src/arrow/compute/kernels/test_util.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/vector_nested_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_validity.cc Outdated
@wesm

wesm commented Jul 14, 2020

Copy link
Copy Markdown
Member

Does this conflict with 1d7d919 which I just merged?

@pitrou

pitrou commented Jul 14, 2020

Copy link
Copy Markdown
MemberAuthor

No, it's rebased already.

@pitrou

pitrou commented Jul 14, 2020

Copy link
Copy Markdown
MemberAuthor

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

@pitrou

pitrou commented Jul 14, 2020

Copy link
Copy Markdown
MemberAuthor

I think this can be merged. Any concerns?

@wesm

wesm commented Jul 14, 2020

Copy link
Copy Markdown
Member

Nope. +1 here

@pitrou
pitrou deleted the ARROW-9390-compute-func-names branch July 14, 2020 16:05
nealrichardson
nealrichardson reviewed Jul 14, 2020
View reviewed changes

@nealrichardsonnealrichardson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment threadcpp/src/arrow/compute/kernels/scalar_string.cc
MakeUnaryStringBatchKernel<AsciiLower>("ascii_lower", registry);

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

@nealrichardsonnealrichardsonJul 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@pitroupitrouJul 14, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

@wesmwesmJul 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can make this change today unless someone else wants to

@pitroupitrouJul 14, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Also ascii_is_alnum, etc. ;-)

Comment threadcpp/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);

@nealrichardsonnealrichardsonJul 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@pitroupitrouJul 14, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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...)

@pitroupitrouJul 14, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Perhaps someone else wants to think about a convention?

pitrou added a commit to pitrou/arrow that referenced this pull request Jul 14, 2020
@pitrou
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.
@pitroupitrou mentioned this pull request Jul 14, 2020
Closed
pitrou added a commit that referenced this pull request Jul 14, 2020
@pitrou
ARROW-9390: [Doc] Add missing file
bfd2568
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>
@asfimportasfimport mentioned this pull request Jul 14, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xhochyxhochyxhochy left review comments
@wesmwesmwesm left review comments
@nealrichardsonnealrichardsonnealrichardson left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pitrou@jorisvandenbossche@wesm@xhochy@nealrichardson