GH-41558: [C++] Improve fixed_width_test_util.h #41575

llama90 · 2024-05-07T15:01:11Z

Rationale for this change

Improve the fixed_width_test_util.h.

What changes are included in this PR?

Move the fixed_width_test_util.h to arrow/testing
Divide the fixed_width_test_util to .cc and .h
Remove unused headers

Are these changes tested?

Yes

Are there any user-facing changes?

No

GitHub Issue: [C++] Move arrow/util/fixed_width_test_util.h to arrow/testing #41558

github-actions · 2024-05-07T15:01:41Z

⚠️ GitHub issue #41558 has been automatically assigned in GitHub to PR creator.

llama90 · 2024-05-07T15:07:20Z

@felipecrv Hello.

Is this what you were thinking?

I would appreciate it if you could review it when you have time. Thank you.

felipecrv · 2024-05-07T18:44:29Z

cpp/src/arrow/testing/fixed_width_test_util.h

-#include <cstddef>
-#include <cstdint>
-#include <memory>
-#include <vector>
-
-#include "arrow/array/builder_primitive.h"
-#include "arrow/builder.h"
-#include "arrow/type.h"
-#include "arrow/util/checked_cast.h"
-


I meant reducing the code in the header. We should keep the necessary includes.

The class should only declare the public function members here and all the private members and template code can be defined in the .cc file (to be created) outside of the class.

See how chunk_resolver.h/cc does it reducing the bloat in the header:

arrow/cpp/src/arrow/chunk_resolver.cc

Lines 44 to 45 in 51689a0

template <typename T>

inline std::vector<int64_t> MakeChunksOffsets(const std::vector<T>& chunks) {

Ah, so I need to organize the code according to C++ conventions. I should include only the declarations in the .h file and put the implementations in the .cc file.

I will refer to the code you shared, organize it accordingly, and then upload it.

Thank you for your guidance.

github-actions · 2024-05-07T20:48:39Z

⚠️ GitHub issue #41558 has been automatically assigned in GitHub to PR creator.

github-actions · 2024-05-12T11:18:25Z

⚠️ GitHub issue #41558 has been automatically assigned in GitHub to PR creator.

github-actions · 2024-05-12T11:37:09Z

⚠️ GitHub issue #41558 has been automatically assigned in GitHub to PR creator.

cpp/src/arrow/testing/fixed_width_test_util.h

felipecrv · 2024-05-14T20:58:10Z

cpp/src/arrow/testing/fixed_width_test_util.h

@@ -120,84 +170,6 @@ class NestedListGenerator {
    }
    return nested_builder->Finish();
  }
-


You should have the definition of NestedListArray in the .cc as well.

llama90 · 2024-05-15T12:07:50Z

@felipecrv Hello, and thank you for your review.

I have applied the changes as advised, but I am a bit unsure about one aspect, particularly because I am not very familiar with template functions.

Should the VisitAllNestedListConfigurations template function also be moved to an anonymous namespace? It is directly called from the test function (vector_selection_test.cc), so I have kept it as it currently is.

Additionally, I made modifications to the Makefile to accommodate the Windows CI process. Could you please confirm if this approach is correct?

I would greatly appreciate your advice on these matters.

Thank you!

felipecrv · 2024-05-15T14:29:28Z

Should the VisitAllNestedListConfigurations template function also be moved to an anonymous namespace? It is directly called from the test function (vector_selection_test.cc), so I have kept it as it currently is.

Sorry, I missed that it was called from others. We can make it a non-template by using std::function<...> instead of inlining the lambda expression. This is not performance-critical code, so it's not gonna be a problem:

-  /// \tparam Visit a function type with signature
-  ///     void(const std::shared_ptr<DataType>& inner_type,
-  ///          const std::vector<int>& list_sizes)
-  template <class Visit>
   static void VisitAllNestedListConfigurations(
-      const std::vector<std::shared_ptr<DataType>>& inner_value_types, Visit&& visit,
+      const std::vector<std::shared_ptr<DataType>>& inner_value_types,
+      const std::function<void(const std::shared_ptr<DataType>& inner_type,
+                               const std::vector<int>& list_sizes)> &visit,
       int max_depth = 3, int max_power_of_2_size = 32) {

Additionally, I made modifications to the Makefile to accommodate the Windows CI process. Could you please confirm if this approach is correct?

We avoid relative paths in CMakeLists.txt because that gets very confusing very quickly. You should list this new .cc file in

arrow/cpp/src/arrow/CMakeLists.txt

Lines 637 to 643 in 2ca9ad2

    
           set(ARROW_TESTING_SRCS 
        
               io/test_common.cc 
        
               ipc/test_common.cc 
        
               testing/gtest_util.cc 
        
               testing/random.cc 
        
               testing/generator.cc 
        
               testing/util.cc)

The trick to find where to list new .cc files is to grep for .cc files in the same directory as yours. Things could be more complicated than that, but it usually works.

llama90 · 2024-05-16T10:24:23Z

I think I applied everything you suggested:

Converted the template function to a regular function.
Modified to not include relative paths in the Makefile (this was a problem due to a missing macro as ARROW_TESTING_EXPORT).

Additionally, I want to express my gratitude for your patience and thorough review. It has been instrumental in completing this PR.

Thank you! cc @felipecrv

felipecrv

Additionally, I want to express my gratitude for your patience and thorough review. It has been instrumental in completing this PR.

Thank you for not giving up ;) C++ is weird.

I added comments about includes, but other than that everything is looking very good.

felipecrv · 2024-05-17T00:53:07Z

cpp/src/arrow/testing/fixed_width_test_util.cc

+// specific language governing permissions and limitations
+// under the License.
+
+#include "fixed_width_test_util.h"


The linter is complaining about this line because includes should be fully qualified based on the cpp/src/ root, so this should be "arrow/testing/fixed_width_test_util.h"

With the removal of include bloat in the header, you should also add all the includes for things referred from this file:

"arrow/array/builder_primitive.h" for NumericBuilder and ArrowBuilder

"arrow/array/builder_base.h" for MakeBuilder [1]

<cstdint>

<limits>

<vector>

<functional>

"arrow/type.h"

[1] my header had an include to arrow/builder.h, but that was unnecessary now that I thought more about it

felipecrv · 2024-05-17T01:01:28Z

cpp/src/arrow/testing/fixed_width_test_util.h

+#include <vector>
+
+#include "arrow/builder.h"
+#include "arrow/testing/visibility.h"


We should try to "include what you use" [1] as much as possible. Even when headers that we need are already included by other headers.

<memory>

<functional>

<cstdint>

"arrow/type.h" for DataType (complete type is needed for the pointers and vector declarations)

"arrow/type_fwd.h" forward-declares ArrayBuilder (which we don't need to be a complete type in the header because we only pointers/references to it in this header)

With these included, you should be able to remove "arrow/builder.h" which is a very expensive header.

[1] https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md

felipecrv · 2024-05-17T01:12:55Z

cpp/src/arrow/testing/fixed_width_test_util.cc

+template Status AppendNumeric<Int8Type>(ArrayBuilder* builder, int64_t* next_value);
+template Status AppendNumeric<Int16Type>(ArrayBuilder* builder, int64_t* next_value);
+template Status AppendNumeric<Int32Type>(ArrayBuilder* builder, int64_t* next_value);
+template Status AppendNumeric<Int64Type>(ArrayBuilder* builder, int64_t* next_value);


You don't need these (in this context) because since this template is in an anon namespace the compiler will only instantiate the 4 instances that are referred to in this compilation unit.

…regular function

llama90 · 2024-05-17T13:52:49Z

@felipecrv I applied your suggestions

Thank you for review :)

conbench-apache-arrow · 2024-05-18T03:25:32Z

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 7aff9d5.

There was 1 benchmark result indicating a performance regression:

Commit Run on ursa-i9-9960x at 2024-05-18 00:39:03Z
- dataset-selectivity (Python) with dataset=nyctaxi_multi_ipc_s3, selectivity=100%

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change Improve the `fixed_width_test_util.h`. ### What changes are included in this PR? - Move the `fixed_width_test_util.h` to `arrow/testing` - Divide the `fixed_width_test_util` to `.cc` and `.h` - Remove unused headers ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#41558 Authored-by: Hyunseok Seo <hsseo0501@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

github-actions bot added Component: C++ awaiting review Awaiting review labels May 7, 2024

felipecrv requested changes May 7, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 7, 2024

kou changed the title ~~GH-41558: Move and update fixed_width_test_util.h~~ GH-41558: [C++] Move and update fixed_width_test_util.h May 7, 2024

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 12, 2024

llama90 changed the title ~~GH-41558: [C++] Move and update fixed_width_test_util.h~~ GH-41558: [C++] Improve fixed_width_test_util.h May 12, 2024

llama90 force-pushed the ARROW-41558 branch from 25b9f22 to a6d210e Compare May 12, 2024 13:20

felipecrv requested changes May 14, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels May 14, 2024

llama90 force-pushed the ARROW-41558 branch 2 times, most recently from 75ed690 to d4961bc Compare May 15, 2024 11:50

felipecrv requested changes May 17, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels May 17, 2024

llama90 force-pushed the ARROW-41558 branch 2 times, most recently from 8a3f0b5 to f76416f Compare May 17, 2024 10:10

llama90 added 13 commits May 17, 2024 19:38

update fixed_width_test_util.h

82e54c9

divide fixed_width_test_util to .cc and .h

c3f1d70

update makefile for window

26f3fd1

define 'NestedListArray' in the .cc

ad04b80

define 'AppendNestedList' in the .cc

6293e02

update 'AppendNumeric' to an an anonymous namespace in .cc

2b22faa

update add_library to remove 'fixed_width_test_util.cc' in CMakeList.txt

366a0ee

update header

6aaa953

rollback makefile for window

c43fd90

convert 'VisitAllNestedListConfigurations' function from template to …

f8d9fa8

…regular function

rollback makefile

dbbbb58

add MACRO for window

8a4e206

update headers as per review suggestions

5966908

llama90 force-pushed the ARROW-41558 branch from f76416f to 5966908 Compare May 17, 2024 10:38

add checked_cast header

934bee1

llama90 requested a review from felipecrv May 17, 2024 13:52

felipecrv approved these changes May 17, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 17, 2024

felipecrv merged commit 7aff9d5 into apache:main May 17, 2024
37 of 38 checks passed

felipecrv removed the awaiting merge Awaiting merge label May 17, 2024

felipecrv mentioned this pull request May 17, 2024

[C++] Move arrow/util/fixed_width_test_util.h to arrow/testing #41558

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41558: [C++] Improve fixed_width_test_util.h #41575

GH-41558: [C++] Improve fixed_width_test_util.h #41575

llama90 commented May 7, 2024 •

edited

github-actions bot commented May 7, 2024

llama90 commented May 7, 2024

felipecrv May 7, 2024

llama90 May 8, 2024

github-actions bot commented May 7, 2024

github-actions bot commented May 12, 2024

github-actions bot commented May 12, 2024

felipecrv May 14, 2024

llama90 commented May 15, 2024

felipecrv commented May 15, 2024 •

edited

llama90 commented May 16, 2024

felipecrv left a comment

felipecrv May 17, 2024

felipecrv May 17, 2024

felipecrv May 17, 2024

felipecrv May 17, 2024

llama90 commented May 17, 2024

conbench-apache-arrow bot commented May 18, 2024

	template <typename T>
	inline std::vector<int64_t> MakeChunksOffsets(const std::vector<T>& chunks) {

GH-41558: [C++] Improve fixed_width_test_util.h #41575

GH-41558: [C++] Improve fixed_width_test_util.h #41575

Conversation

llama90 commented May 7, 2024 • edited

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented May 7, 2024

llama90 commented May 7, 2024

felipecrv May 7, 2024

Choose a reason for hiding this comment

llama90 May 8, 2024

Choose a reason for hiding this comment

github-actions bot commented May 7, 2024

github-actions bot commented May 12, 2024

github-actions bot commented May 12, 2024

felipecrv May 14, 2024

Choose a reason for hiding this comment

llama90 commented May 15, 2024

felipecrv commented May 15, 2024 • edited

llama90 commented May 16, 2024

felipecrv left a comment

Choose a reason for hiding this comment

felipecrv May 17, 2024

Choose a reason for hiding this comment

felipecrv May 17, 2024

Choose a reason for hiding this comment

felipecrv May 17, 2024

Choose a reason for hiding this comment

felipecrv May 17, 2024

Choose a reason for hiding this comment

llama90 commented May 17, 2024

conbench-apache-arrow bot commented May 18, 2024

llama90 commented May 7, 2024 •

edited

felipecrv commented May 15, 2024 •

edited