Meta-programming tools and arrow types traits support. #49

Klaim · 2024-03-27T17:31:01Z

Fix #67
Fix #68
Part of the effort for #15

added various metaprogramming basic algorithms and tools;
added concepts to check arrow-supported types;
added arrow_traits to extract information about arrow-supported types, including custom ones;
related tests.
added float16_t support, using copy-pasted dependency in C++20, standard library in C++23

include/sparrow/data_traits.hpp

include/sparrow/data_type.hpp

include/sparrow/mp_utils.hpp

jjerphan

A first pass.

jjerphan · 2024-03-28T07:29:22Z

include/sparrow/data_type.hpp

+    ,   float32_t
+    ,   float64_t
+    ,   std::string
+    //,   std::vector<std::byte> // REVIEW should this be uint8_t? char? buffer<unit8_t>?


It's true that we have used uint8_t over std::byte for the buffer.

Is uint8_t use for interoperability with C, or are there other reason not to use std::byte?

Also, if we use uint8_t we must:

static_assert(CHAR_BIT == 8);

The main "classic" issue is that because std::byte has a strong type, not implicitly converted to other similar types, it's not automatically compatible with functions working with char or char-like types, in particular when interfacing with C. Lots of explicit casts ensures. Not a big problem if you like correctness but it does obfuscate the code. It's the main criticism I'm aware of.

In this library, I'm not sure what would be best, but the C api kinds of forces us with at least some of the types?

Also, if we use uint8_t we must:

hmm actually shouldnt we always check that? I'll add it.

What do you think, @JohanMabille?

The C Api uses void* for the buffers, so we'll have to cast anyway. I would actually be in favor of replacing the uint8_t with byte in the array_data buffers.

Ok I went with an alias byte_t so that we can change that easilly, and added vector<byte_t> and I suppose we'll see how it goes from here.

include/sparrow/details/3rdparty/README.md

include/sparrow/details/3rdparty/float16_t.hpp

include/sparrow/data_type.hpp

test/test_mpl.cpp

include/sparrow/data_traits.hpp

include/sparrow/data_type.hpp

test/test_mpl.cpp

test/test_traits.cpp

jjerphan

Looks neat to me!

I have few comments and questions.

include/sparrow/data_traits.hpp

include/sparrow/data_type.hpp

include/sparrow/mp_utils.hpp

include/sparrow/data_type.hpp

jjerphan · 2024-04-05T12:11:32Z

include/sparrow/data_type.hpp

+    ,   float32_t
+    ,   float64_t
+    ,   std::string
+    //,   std::vector<std::byte> // REVIEW should this be uint8_t? char? buffer<unit8_t>?


What do you think, @JohanMabille?

test/test_mpl.cpp

test/test_traits.cpp

include/sparrow/mp_utils.hpp

- added various metaprogramming basic algorithms and tools; - added concepts to check arrow-supported types; - added `arrow_traits` to extract information about arrow-supported types, including custom ones; - related tests. - added `float16_t` support, using copy-pasted dependency in C++20, standard library in C++23

Co-authored-by: Julien Jerphanion <[email protected]>

…valuate` is enough.

…pe-traits.

Co-authored-by: Julien Jerphanion <[email protected]> Co-authored-by: Alexis Placet <[email protected]>

Co-authored-by: Julien Jerphanion <[email protected]>

test/test_mpl.cpp

include/sparrow/data_type.hpp

JohanMabille · 2024-04-08T13:37:29Z

LGTM except the minor comment I left.

JohanMabille · 2024-04-08T13:55:52Z

include/sparrow/mp_utils.hpp

+            or
+                requires(std::decay_t<P> predicate)
+                {
+                    { predicate(std::type_identity_t<T>{}) } -> std::convertible_to<bool>;


I don't remember why we need to support both type_identity_t and typelist.Would the support of type_identity be enough? It looks like predicates will be evaluated over types in a typelist via the evaluate function, resulting in a pattern like (evaluate<T> OP ... OP init); therefore evaluate could accept type_identity_t only and that would simplify the code. But as always when I have this feeling, I probably missed something.

I don't remember why we need to support both type_identity_t and typelist.

I started with typelist because that's what the algorithms in this PR work with as argument, to not deviate too far. The support for type_identity you suggested simply allows for code taht dont depend on sparrow to still add new type-predicates as callable objects.

Would the support of type_identity be enough?

Both are fine to me, or do you see a cost I'm not seeing?

It looks like predicates will be evaluated over types in a typelist via the evaluate function, resulting in a pattern like (evaluate OP ... OP init); therefore evaluate could accept type_identity_t only and taht would simplify the code. But as always when I have this feeling, I probably issed something.

No you're right, both typelist and type_identity works here so I support both and other than the potential extension without being dependent on sparrow, it would indeed simplify some of the code.

Not sure what to do here. I have a slight preference toward supporting "anything that can act as a type wrapper" so that's why I switched to using the mpl::type_wrapper concept instead of being too specific. The implementations of the algorithms could be rewritten with any type wrappers.

Choices. :)

Both are fine to me, or do you see a cost I'm not seeing?

Not really, it was really for simplifying the code. Also concepts with or clauses don't subsume, but I guess it's not really importnat here since we'll probably won'r refine the callable_type_predicate concept.

I would say let's keep it as is for now, and use it. And we'll see later if we can really simplify it (if that's really worth it, and it it does not break any code using it).

jjerphan

LGTM.

Minor suggestion which need not be adressed: we could reference the thread of discussions so that we can more easily refer to them in the future.

Klaim requested review from JohanMabille and jjerphan March 27, 2024 18:05

JohanMabille reviewed Mar 27, 2024

View reviewed changes

jjerphan reviewed Mar 28, 2024

View reviewed changes

Klaim force-pushed the klaim/arrow_traits branch from 8343468 to 449f206 Compare April 2, 2024 16:15

Alex-PLACET reviewed Apr 3, 2024

View reviewed changes

include/sparrow/data_type.hpp Outdated Show resolved Hide resolved

test/test_mpl.cpp Outdated Show resolved Hide resolved

test/test_traits.cpp Outdated Show resolved Hide resolved

jjerphan reviewed Apr 5, 2024

View reviewed changes

Klaim and others added 13 commits April 5, 2024 16:20

refactor common native types traits

910bfa1

fixed documentation

30bc8ec

Typo fix

ec61b04

Co-authored-by: Julien Jerphanion <[email protected]>

Use permalink when referencing 3rd party source

fc27ad0

Co-authored-by: Julien Jerphanion <[email protected]>

Improved 3rd party rational

e043ef8

Co-authored-by: Julien Jerphanion <[email protected]>

type predicate support (wip)

c1e2f7e

No common concept for type-predicates, 2 concepts and overloads of `e…

9e2f3c0

…valuate` is enough.

experimenting with enabling easy support for standard type predicates

73966ff

Completed support for allowing type-predicates built from standard ty…

1ce1c78

…pe-traits.

Apply suggestions from code review

4b7d2db

Co-authored-by: Julien Jerphanion <[email protected]> Co-authored-by: Alexis Placet <[email protected]>

fixed rebasing issue

e6d52b9

removed unused includes of doctest

67b4769

Klaim force-pushed the klaim/arrow_traits branch from b82e382 to 67b4769 Compare April 5, 2024 14:32

Klaim and others added 5 commits April 5, 2024 16:47

Applied review comments

f98b986

removed obsolete commented code

1bd7f2b

Apply suggestions from code review

2efac9c

Co-authored-by: Julien Jerphanion <[email protected]>

fixed issues pointed by clang and added related tests

e3e53bb

fixed CHAR_BIT missing climit header

6125e1c

Klaim commented Apr 7, 2024

View reviewed changes

test/test_mpl.cpp Show resolved Hide resolved

JohanMabille approved these changes Apr 8, 2024

View reviewed changes

include/sparrow/data_type.hpp Outdated Show resolved Hide resolved

JohanMabille reviewed Apr 8, 2024

View reviewed changes

Klaim added 3 commits April 8, 2024 17:05

Use std::byte for now to represent bytes.

5dccd5c

clarify mpl tests

ca28676

Fixed missing template parameter name in doc

f139fab

Klaim marked this pull request as ready for review April 8, 2024 15:21

jjerphan approved these changes Apr 8, 2024

View reviewed changes

Klaim merged commit 2a07414 into man-group:main Apr 9, 2024
20 checks passed

jjerphan mentioned this pull request Apr 9, 2024

Implement typed_array #65

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Meta-programming tools and arrow types traits support. #49

Meta-programming tools and arrow types traits support. #49

Klaim commented Mar 27, 2024 •

edited by JohanMabille

Loading

jjerphan left a comment

jjerphan Mar 28, 2024

Klaim Mar 28, 2024

jjerphan Apr 5, 2024

JohanMabille Apr 8, 2024

Klaim Apr 8, 2024

jjerphan left a comment •

edited

Loading

jjerphan Apr 5, 2024

JohanMabille commented Apr 8, 2024

JohanMabille Apr 8, 2024 •

edited

Loading

Klaim Apr 8, 2024

JohanMabille Apr 8, 2024

jjerphan left a comment

Meta-programming tools and arrow types traits support. #49

Meta-programming tools and arrow types traits support. #49

Conversation

Klaim commented Mar 27, 2024 • edited by JohanMabille Loading

jjerphan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjerphan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanMabille commented Apr 8, 2024

JohanMabille Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjerphan left a comment

Choose a reason for hiding this comment

Klaim commented Mar 27, 2024 •

edited by JohanMabille

Loading

jjerphan left a comment •

edited

Loading

JohanMabille Apr 8, 2024 •

edited

Loading