ARROW-17289: [C++] Add type category membership checks by rtpsw · Pull Request #13783 · apache/arrow

rtpsw · 2022-08-02T20:08:06Z

See https://issues.apache.org/jira/browse/ARROW-17289

github-actions · 2022-08-02T20:08:33Z

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

rtpsw · 2022-08-02T21:13:04Z

cc @icexelloss, @westonpace

pitrou

I'm extremely lukewarm about this. In any case, the implementation is needlessly inefficient.

pitrou · 2022-08-03T12:14:47Z

This is really an inefficient way to do it instead of simply switching based on the id().

pitrou · 2022-08-03T12:16:28Z

I'm leaning towards -1 on adding these APIs. There are similar APIs, with different names and inlined, that take a type id. It's not significantly to write IsSignedIntType(type) rather than is_signed_integer(type->id()).

In addition, the signature is inefficient as this is always copying a shared_ptr (implying an atomic reference increment). Instead this could just take a const DataType&.

It turns out the set of *Type() functions does not correspond to the set of is_* ones. This can be really confusing to the user; for example, one could easily incorrectly assume that PrimtiveTypes() corresponds to is_primitive. So, I think some clean up is warranted around this code, while considering backwards compatibility.

I'll soon add a commit here that shows the is_* functions I needed to add to obtain correspondence. However, I don't like the mess this makes of the function names, so I'd appreciate a suggestion here.

pitrou · 2022-08-03T12:18:10Z

It sounds like it would be better to document the existing predicates, rather than add new ones with different names.

rtpsw · 2022-08-03T18:11:08Z

It sounds like it would be better to document the existing predicates, rather than add new ones with different names.

The existing set of predicates is both confusing and incomplete, so I'd argue for both documenting and adding predicates. I'm not sure what to do with the names, considering backwards compatibility, though. Suppose we agree that a predicate corresponding to PrimitiveTypes should be added, since is_primitive is not the one, what should be the name of this predicate? There should be some naming convention here; perhaps *Types() should have a corresponding check_*(type) predicate.

rtpsw · 2022-08-04T09:12:38Z

I added docs that clarify the meaning of and correspondence between *Types() and Is*Type() functions.

pitrou · 2022-08-04T09:25:26Z

The existing set of predicates is both confusing and incomplete, so I'd argue for both documenting and adding predicates

Hmm... can you point out the causes for confusion? Perhaps there's a way to improve that.

Same for incompleteness: is the set of Type::type predicates incomplete? if so, we should add the missing and desired predicates.

Suppose we agree that a predicate corresponding to PrimitiveTypes should be added, since is_primitive is not the one

Why is is_primitive not the one? It seems like it is returning the desired information, is it not? (is it missing some types perhaps?)

rtpsw · 2022-08-04T10:05:00Z

Hmm... can you point out the causes for confusion? Perhaps there's a way to improve that.

Same for incompleteness: is the set of Type::type predicates incomplete? if so, we should add the missing and desired predicates.

Sure. Check out this commit:

is_numeric is missing and I added it.
is_primitive exists but does not correspond to PrimitiveTypes(), which is confusing. I added is_primitive_like which does, though I'm not sold on the name.
I added the missing is_binary, is_string, is_temporal, and is_interval.
There are other is_*() primitives that do not correspond to any *Types() function - is_base_binary_like is one example.

So, part of the confusion is the unexpected non-correspondence of is_primitive and part is due to missing and extra predicates with respect to the set of *Types() functions. In this PR, I extended the set of is_*() predicates to cover the correspondences and added Is*Type() predicates that correspond to *Types() functions as expected, while maintaining backwards-compatibility, and I added docs to clarify the meaning and correspondences.

Why is is_primitive not the one? It seems like it is returning the desired information, is it not? (is it missing some types perhaps?)

As compared to PrimitiveTypes() (and is_primitive_like() that I added), is_primitive is missing NA, BINARY, STRING, LARGE_BINARY and LARGE_STRING while it adds DURATION, INTERVAL_MONTHS, INTERVAL_MONTH_DAY_NANO, and INTERVAL_DAY_TIME.

pitrou · 2022-08-04T12:03:20Z

Ok, so there are two different areas here:

complete and/or fix the set of predicates operating on a Type::type parameter
add a separate set of predicates operating on a const DataType& parameter

I think the former (case number 1 above) is the most interesting to solve, because it directly affects readability.

Would you like to defer number 2 to a separate JIRA (and potentially PR, if people agree it's a worthwhile thing to do)?

pitrou · 2022-08-04T12:04:47Z

Also, for the record, functions such as PrimitiveTypes() were added for testing originally, and they were exposed publicly because there didn't seem to be a strong reason against it (the main use case is to be able to write testing loops or parameterized tests e.g. for (const auto& type : PrimitiveTypes()) ...).

pitrou · 2022-08-04T12:11:38Z

Now to answer the problems with the current predicates:

+1 to adding is_numeric, is_binary, is_string, is_temporal and is_interval
for is_primitive: is "primitive" any type that's not nested nested/compound; whether or Type::NA should be included is a bit contentious, but otherwise all temporals, numerics (including decimals) and binary-likes should probably be included
I think is_primitive_like should probably be avoided; granted, we have other is_*_like functions, but they turn out to be confusing, so we should probably not reuse that pattern
therefore I would be in favor of changing is_primitive as outlined above (meaning a slight compatibility break unfortunately)

Also note that the is_* predicates can handle parameterized types while the *Types() functions by essence cannot really handle them fully if there is a large or infinite number of parametrizations possible.

Does that make sense?

Also @lidavidm what do you think?

lidavidm · 2022-08-04T12:18:47Z

Note #13753 removes the *Types() anyways.

I'm -1 on the PR as is, but I think we should add the missing is_*(Type::type id) functions, and I think it's fine to also add overloads that take const DataType&. (This is probably overkill, but those who rely on IDE autocompletion may also appreciate DataType::is_integer etc.)

"primitive" is hard to define and it depends on what the user is after. I might favor a set of more specific predicates like the ones we have for template metaprogramming, e.g. has_c_type (which I think is roughly what the proposed is_primitive is: NA and nested types don't have a C type, but binary-likes do).

rtpsw · 2022-08-04T12:45:23Z

OK, so I'll avoid changes around the "primitive" functions and fix this PR to only:

add the missing is_numeric, is_binary, is_string, is_temporal and is_interval predicates
add overloads (same names) for all is_* predicates that accept const DataType
test these predicates

rtpsw · 2022-08-04T18:52:04Z

Anyone knows what the errors in the Dev jobs mean?

lidavidm · 2022-08-04T18:59:50Z

I think you can ignore those, some files need to be bumped post-release

lidavidm · 2022-08-05T17:15:02Z

Would is_not_nested or something be clearer here? "primitive-like" is a little confusing (though, I suppose that then raises the question of why fixed-width binary and decimal are also missing here)

I forgot to remove this predicate and I'll do so soon. It was only added to correspond to some *Types() function, all of which are going to be removed.

Side note: the existing (pre-PR) code has an is_nested predicate, though it is not the complement of the function discussed here, e.g., due to timestamp and interval types.

pitrou

Thanks for the update! I posted a couple suggestions but this looks good overall.

rtpsw · 2022-08-15T18:09:30Z

@pitrou, is this good to go?

Co-authored-by: Antoine Pitrou <[email protected]>

pitrou · 2022-08-17T13:57:02Z

Sorry for the delay @rtpsw . I rebased from master and did a couple of very minor changes.

pitrou

LGTM!

pitrou · 2022-08-17T13:57:28Z

Will wait for CI now...

ursabot · 2022-08-17T23:33:09Z

Benchmark runs are scheduled for baseline = cef6894 and contender = f0688d0. f0688d0 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️25.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.46% ⬆️0.44%] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] f0688d01 ec2-t3-xlarge-us-east-2
[Failed] f0688d01 test-mac-arm
[Failed] f0688d01 ursa-i9-9960x
[Finished] f0688d01 ursa-thinkcentre-m75q
[Finished] cef68940 ec2-t3-xlarge-us-east-2
[Finished] cef68940 test-mac-arm
[Failed] cef68940 ursa-i9-9960x
[Failed] cef68940 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

See https://issues.apache.org/jira/browse/ARROW-17289 Lead-authored-by: Yaron Gvili <[email protected]> Co-authored-by: rtpsw <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

github-actions Bot added the Component: C++ label Aug 2, 2022

pitrou reviewed Aug 3, 2022

View reviewed changes

rtpsw requested a review from pitrou August 4, 2022 18:52

lidavidm reviewed Aug 5, 2022

View reviewed changes

pitrou reviewed Aug 8, 2022

View reviewed changes

Comment thread cpp/src/arrow/type_traits.h Resolved

Comment thread cpp/src/arrow/type_traits.h Resolved

Comment thread cpp/src/arrow/type_traits.h Resolved

Comment thread cpp/src/arrow/type_traits.h Resolved

Comment thread cpp/src/arrow/type_traits.h Resolved

rtpsw requested a review from pitrou August 10, 2022 06:44

rtpsw and others added 9 commits August 17, 2022 15:44

ARROW-17289: [C++] Add type category membership checks

8050354

use type id, improve test coverage of types

f38ec91

add docs

c3d4d35

fix type predicates

ba232b7

lint

6669b7f

remove is_primitive_like

99ddf32

Update cpp/src/arrow/type_traits.h

f01a215

Co-authored-by: Antoine Pitrou <[email protected]>

Update cpp/src/arrow/type_traits.h

353b7ab

Co-authored-by: Antoine Pitrou <[email protected]>

Update cpp/src/arrow/type_traits.h

15dcecb

Co-authored-by: Antoine Pitrou <[email protected]>

rtpsw and others added 3 commits August 17, 2022 15:44

Update cpp/src/arrow/type_traits.h

2d79b36

Co-authored-by: Antoine Pitrou <[email protected]>

Update cpp/src/arrow/type_traits.h

21fc1c4

Co-authored-by: Antoine Pitrou <[email protected]>

Minor improvements to docstrings

3c173b5

pitrou force-pushed the ARROW-17289 branch from ac1316c to 3c173b5 Compare August 17, 2022 13:56

pitrou approved these changes Aug 17, 2022

View reviewed changes

pitrou merged commit f0688d0 into apache:master Aug 17, 2022

Conversation

rtpsw commented Aug 2, 2022

Uh oh!

github-actions Bot commented Aug 2, 2022

Uh oh!

rtpsw commented Aug 2, 2022

Uh oh!

pitrou 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!

pitrou commented Aug 3, 2022

Uh oh!

rtpsw commented Aug 3, 2022

Uh oh!

rtpsw commented Aug 4, 2022

Uh oh!

pitrou commented Aug 4, 2022

Uh oh!

rtpsw commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Aug 4, 2022

Uh oh!

pitrou commented Aug 4, 2022

Uh oh!

pitrou commented Aug 4, 2022

Uh oh!

lidavidm commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rtpsw commented Aug 4, 2022

Uh oh!

rtpsw commented Aug 4, 2022

Uh oh!

lidavidm commented Aug 4, 2022

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!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rtpsw commented Aug 15, 2022

Uh oh!

pitrou commented Aug 17, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Aug 17, 2022

Uh oh!

ursabot commented Aug 17, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rtpsw commented Aug 4, 2022 •

edited

Loading

lidavidm commented Aug 4, 2022 •

edited

Loading