Plugged binary layout #312

JohanMabille · 2024-12-17T14:45:55Z

No description provided.

Alex-PLACET · 2024-12-18T09:15:07Z

include/sparrow/utils/vector_view.hpp

+    constexpr std::compare_three_way_result<T>
+    operator<=>(const vector_view<T>& lhs, const vector_view<T>& rhs)
+    {
+        return std::lexicographical_compare_three_way(lhs.begin(), lhs.end(), rhs.begin(), rhs.end());


Hmm if you can use the std here, it's means that we don't need the sparrow::lexicographical_compare_three_way implementation anymore (because we use newer version of Xcode I guess).
We should remove this code in another PR.

Alex-PLACET · 2024-12-18T09:18:00Z

test/test_binary_array.cpp

+        arrow_proxy create_arrow_proxy()
+        {
+            ArrowSchema schema{};
+            ArrowArray array{};
+            const std::vector<size_t> false_bitmap{m_false_bitmap.begin(), m_false_bitmap.end()};
+            test::fill_schema_and_array<std::vector<byte_t>>(schema, array, m_length, m_offset, false_bitmap);
+            return arrow_proxy{std::move(array), std::move(schema)};
+        }


We should try to no use this anymore and always use the convenient constructors.
And in the convenient constructors test, check the arrow_proxy created.

I think this should be done in a dedicated PR where we totally hide the arrow_proxy. As long as the arrow_proxy and the array constructors accepting it are exposed, we need to test them.

Alex-PLACET · 2024-12-18T09:18:25Z

test/test_binary_array.cpp

+                CHECK_EQ(array[0].value(), word0);
+                CHECK_EQ(array[1].value(), word1);
+                CHECK_EQ(array[4].value(), word4);
+            } 


Please check the created arrow_proxy

I think we should not. We should totally hide the arrow_proxy, and have constructors accepting ArrowArray and ArrowSchema only. This should be done in a dedicated PR.

Alex-PLACET · 2024-12-18T09:18:59Z

test/test_binary_array.cpp

+                CHECK_EQ(array[0].has_value(), true);
+                CHECK_EQ(array[1].has_value(), true);
+                CHECK_EQ(array[2].has_value(), false);
+                CHECK_EQ(array[3].has_value(), false);
+                CHECK_EQ(array[4].has_value(), true);


Suggested change

CHECK_EQ(array[0].has_value(), true);

CHECK_EQ(array[1].has_value(), true);

CHECK_EQ(array[2].has_value(), false);

CHECK_EQ(array[3].has_value(), false);

CHECK_EQ(array[4].has_value(), true);

CHECK(array[0].has_value());

CHECK(array[1].has_value());

CHECK_FALSE(array[2].has_value());

CHECK_FALSE(array[3].has_value());

CHECK(array[4].has_value());

Alex-PLACET · 2024-12-18T09:19:11Z

test/test_binary_array.cpp

+                    word4
+                };
+                std::vector<std::size_t> where_nulls{2,3};
+                binary_array array(words, std::move(where_nulls));


Suggested change

binary_array array(words, std::move(where_nulls));

const binary_array array(words, std::move(where_nulls));

Alex-PLACET · 2024-12-18T09:20:54Z

test/test_binary_array.cpp

+                CHECK_EQ(*citer, true);
+                CHECK_EQ(*(++citer), false);
+                CHECK_EQ(*(++citer), true);
+                CHECK_EQ(*(++citer), true);
+                CHECK_EQ(*(++citer), false);
+                CHECK_EQ(*(++citer), true);
+                CHECK_EQ(*(++citer), true);
+                CHECK_EQ(*(++citer), true);
+                CHECK_EQ(*(++citer), true);


Suggested change

CHECK_EQ(*citer, true);

CHECK_EQ(*(++citer), false);

CHECK_EQ(*(++citer), true);

CHECK_EQ(*(++citer), true);

CHECK_EQ(*(++citer), false);

CHECK_EQ(*(++citer), true);

CHECK_EQ(*(++citer), true);

CHECK_EQ(*(++citer), true);

CHECK_EQ(*(++citer), true);

CHECK(*citer);

CHECK_FALSE(*(++citer));

CHECK(*(++citer));

CHECK(*(++citer));

CHECK_FALSE(*(++citer));

CHECK(*(++citer));

CHECK(*(++citer));

CHECK(*(++citer));

CHECK(*(++citer));

codecov-commenter · 2024-12-18T15:01:53Z

Codecov Report

Attention: Patch coverage is 60.71429% with 22 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@3b9e4da). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
..._size_binary_layout/variable_size_binary_array.hpp	55.00%	9 Missing ⚠️
include/sparrow/types/data_type.hpp	50.00%	6 Missing ⚠️
include/sparrow/layout/dispatch.hpp	0.00%	3 Missing ⚠️
src/array_factory.cpp	0.00%	3 Missing ⚠️
src/arrow_interface/arrow_array.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #312   +/-   ##
=======================================
  Coverage        ?   91.84%           
=======================================
  Files           ?       77           
  Lines           ?     5736           
  Branches        ?        0           
=======================================
  Hits            ?     5268           
  Misses          ?      468           
  Partials        ?        0

Flag	Coverage Δ
unittests	`91.84% <60.71%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JohanMabille · 2024-12-19T09:35:08Z

I'll update the builders in a dedicated PR, would be nice to finish to merge the remaining PRs so that we can enable precommit-ci and merge #311

JohanMabille force-pushed the binary_layout branch 3 times, most recently from a22d0a3 to b1f981e Compare December 17, 2024 15:13

Alex-PLACET reviewed Dec 18, 2024

View reviewed changes

JohanMabille added 2 commits December 18, 2024 14:18

Plugged binary layout

9d92aa2

rebase and fix

cd52b2d

JohanMabille force-pushed the binary_layout branch from b1f981e to cd52b2d Compare December 18, 2024 14:40

Alex-PLACET approved these changes Dec 18, 2024

View reviewed changes

Fixed get_arrow_array_bufers

89b8c24

JohanMabille merged commit c015596 into man-group:main Dec 19, 2024
73 of 74 checks passed

JohanMabille deleted the binary_layout branch December 19, 2024 10:39

JohanMabille mentioned this pull request Dec 19, 2024

add LARGE_BINARY and LARGE_STRING #280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugged binary layout #312

Plugged binary layout #312

JohanMabille commented Dec 17, 2024

Alex-PLACET Dec 18, 2024

Alex-PLACET Dec 18, 2024

JohanMabille Dec 18, 2024

Alex-PLACET Dec 18, 2024

JohanMabille Dec 18, 2024 •

edited

Loading

Alex-PLACET Dec 18, 2024

Alex-PLACET Dec 18, 2024

Alex-PLACET Dec 18, 2024

codecov-commenter commented Dec 18, 2024 •

edited by codecov bot

Loading

JohanMabille commented Dec 19, 2024

	binary_array array(words, std::move(where_nulls));
	const binary_array array(words, std::move(where_nulls));

Plugged binary layout #312

Plugged binary layout #312

Conversation

JohanMabille commented Dec 17, 2024

Alex-PLACET Dec 18, 2024

Choose a reason for hiding this comment

Alex-PLACET Dec 18, 2024

Choose a reason for hiding this comment

JohanMabille Dec 18, 2024

Choose a reason for hiding this comment

Alex-PLACET Dec 18, 2024

Choose a reason for hiding this comment

JohanMabille Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Alex-PLACET Dec 18, 2024

Choose a reason for hiding this comment

Alex-PLACET Dec 18, 2024

Choose a reason for hiding this comment

Alex-PLACET Dec 18, 2024

Choose a reason for hiding this comment

codecov-commenter commented Dec 18, 2024 • edited by codecov bot Loading

Codecov Report

JohanMabille commented Dec 19, 2024

JohanMabille Dec 18, 2024 •

edited

Loading

codecov-commenter commented Dec 18, 2024 •

edited by codecov bot

Loading