GH-37906: [Integration][C#] Implement C Data Interface integration testing for C# #37904

pitrou · 2023-09-27T13:29:41Z

Rationale for this change

All Arrow implementations supporting the C Data Interface should also implement integration testing, to ensure proper interoperability.

What changes are included in this PR?

Implement C Data Interface integration testing for C#
Fix bugs in the C Data Interface implementation for C#

Are these changes tested?

Yes, by construction.

Are there any user-facing changes?

No.

Closes: [Integration][C#] Implement C Data Interface integration testing for C# #37906

pitrou · 2023-09-27T14:12:15Z

@CurtHagenlocher @teo-tsirpanis @wjones127

pitrou · 2023-09-27T14:16:54Z

csharp/test/Apache.Arrow.IntegrationTest/JsonFile.cs

    }

    public class JsonSchema
    {
        public List<JsonField> Fields { get; set; }
        public JsonMetadata Metadata { get; set; }
+
+        /// <summary>


Note that all the code below is moved from IntegrationCommand.cs in order for it to be available to the integration testing harness.

pitrou · 2023-09-27T14:25:18Z

csharp/test/Apache.Arrow.IntegrationTest/CDataInterface.cs

+            GC.Collect();
+            // XXX this doesn't seem to give stable and reliable measurements
+            var gcInfo = GC.GetGCMemoryInfo();
+            return gcInfo.PromotedBytes;


I wasn't able to find a formula that produced reliable numbers. Inevitably, the numbers are fluctuating (going up and down). I don't know if the .Net GC is just giving us approximate numbers, or if some internal caches are interfering.

Are we sure that all the tests are using the TestMemoryAllocator (which uses the managed heap)? I suspect some arrays may be allocated using the NativeMemoryAllocator which does not used the managed heap.

Whether or not it's strictly required, I've developed a habit of something like

for (int i = 0; i < 3; i++) {
GC.Collect();
GC.WaitForPendingFinalizers();
}

and this seems to sometimes make a difference.

I also don't know what kind of effect pythonnet would have on this. There may be Python objects waiting for a Python GC that are keeping .NET objects alive.

Are we sure that all the tests are using the TestMemoryAllocator (which uses the managed heap)? I suspect some arrays may be allocated using the NativeMemoryAllocator which does not used the managed heap.

Hmm, I'm discovering the C# codebase, but this is what I get:

the JSON reader in Apache.Arrow.IntegrationTest just uses the default memory allocator when calling ArrowBuffer.Builder.Build()

the TestMemoryAllocator doesn't implement IOwnableAllocation, which is required by CArrowArrayExporter for all exported buffers

In any case, however, the problem I'm mentioning in the comment above (unstable measurements) can be observed even on the Schema tests, which shouldn't invoke an external allocator.

I also don't know what kind of effect pythonnet would have on this. There may be Python objects waiting for a Python GC that are keeping .NET objects alive.

That would seem surprising, but I can try a gc.collect() on the Python side as well...

Okay, none of your suggestions works unfortunately.

My knowledge of the GC is less like science and more like voodoo. I'm also not familiar with this API. That said, PromotedBytes seems like it's probably not the right choice here and I would expect HeapSizeBytes to be the value that's actually wanted.

EDIT: Or for the total managed heap, GC.GetTotalMemory()

Ah, but I also tried GC.GetTotalMemory(), HeapSizeBytes and also HeapSizeBytes - FragmentedBytes. None gave stable values.

wjones127

These changes look good to me.

But look at the test logs, I'm unsure of where we are setting these tests to skip: https://github.com/apache/arrow/actions/runs/6327155294/job/17182356143?pr=37904#step:8:14519

Some of them I would have thought would run.

pitrou · 2023-09-27T15:50:42Z

But look at the test logs, I'm unsure of where we are setting these tests to skip: https://github.com/apache/arrow/actions/runs/6327155294/job/17182356143?pr=37904#step:8:14519

These are existing skips that also propagate to the C Data Interface testing:

arrow/dev/archery/archery/integration/datagen.py

Lines 1787 to 1875 in 2864870

    
           file_objs = [ 
        
               generate_primitive_case([], name='primitive_no_batches'), 
        
               generate_primitive_case([17, 20], name='primitive'), 
        
               generate_primitive_case([0, 0, 0], name='primitive_zerolength'), 
        
               generate_primitive_large_offsets_case([17, 20]) 
        
               .skip_tester('C#') 
        
               .skip_tester('JS'), 
        
               generate_null_case([10, 0]), 
        
               generate_null_trivial_case([0, 0]), 
        
               generate_decimal128_case(), 
        
               generate_decimal256_case() 
        
               .skip_tester('JS'), 
        
               generate_datetime_case(), 
        
               generate_duration_case() 
        
               .skip_tester('C#') 
        
               .skip_tester('JS'),  # TODO(ARROW-5239): Intervals + JS 
        
               generate_interval_case() 
        
               .skip_tester('C#') 
        
               .skip_tester('JS'),  # TODO(ARROW-5239): Intervals + JS 
        
               generate_month_day_nano_interval_case() 
        
               .skip_tester('C#') 
        
               .skip_tester('JS'), 
        
               generate_map_case() 
        
               .skip_tester('C#'), 
        
               generate_non_canonical_map_case() 
        
               .skip_tester('C#') 
        
               .skip_tester('Java')  # TODO(ARROW-8715) 
        
               # Canonical map names are restored on import, so the schemas are unequal 
        
               .skip_format(SKIP_C_SCHEMA, 'C++'), 
        
               generate_nested_case(), 
        
               generate_recursive_nested_case(), 
        
               generate_nested_large_offsets_case() 
        
               .skip_tester('C#') 
        
               .skip_tester('JS'), 
        
               generate_unions_case(), 
        
               generate_custom_metadata_case() 
        
               .skip_tester('C#'), 
        
               generate_duplicate_fieldnames_case() 
        
               .skip_tester('C#') 
        
               .skip_tester('JS'), 
        
               generate_dictionary_case() 
        
               .skip_tester('C#'), 
        
               generate_dictionary_unsigned_case() 
        
               .skip_tester('C#') 
        
               .skip_tester('Java'),  # TODO(ARROW-9377) 
        
               generate_nested_dictionary_case() 
        
               .skip_tester('C#') 
        
               .skip_tester('Java'),  # TODO(ARROW-7779) 
        
               generate_run_end_encoded_case() 
        
               .skip_tester('C#') 
        
               .skip_tester('Java') 
        
               .skip_tester('JS') 
        
               .skip_tester('Rust'), 
        
               generate_binary_view_case() 
        
               .skip_tester('C++') 
        
               .skip_tester('C#') 
        
               .skip_tester('Go') 
        
               .skip_tester('Java') 
        
               .skip_tester('JS') 
        
               .skip_tester('Rust'), 
        
               generate_extension_case() 
        
               .skip_tester('C#') 
        
               # TODO: ensure the extension is registered in the C++ entrypoint 
        
               .skip_format(SKIP_C_SCHEMA, 'C++') 
        
               .skip_format(SKIP_C_ARRAY, 'C++'), 
        
           ]

CurtHagenlocher · 2023-09-27T15:50:48Z

Neat! I'd actually started looking at implementing this yesterday and didn't see that there was a route through Python to test in this fashion so I'd started working on a separate project to use AOT compilation and generate a native DLL for this purpose.

csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs

pitrou · 2023-10-02T12:35:27Z

@eerhardt Would you like to review this PR soon? Otherwise I'm inclined to merge as it has already been approved by @CurtHagenlocher and @wjones127 .

csharp/test/Apache.Arrow.IntegrationTest/CDataInterface.cs

…ion testing for C#

conbench-apache-arrow · 2023-10-02T20:36:18Z

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…ion testing for C# (apache#37904) ### Rationale for this change All Arrow implementations supporting the C Data Interface should also implement integration testing, to ensure proper interoperability. ### What changes are included in this PR? * Implement C Data Interface integration testing for C# * Fix bugs in the C Data Interface implementation for C# ### Are these changes tested? Yes, by construction. ### Are there any user-facing changes? No. * Closes: apache#37906 Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

This comment was marked as resolved.

Sign in to view

github-actions bot added Component: C# awaiting review Awaiting review labels Sep 27, 2023

pitrou changed the title ~~[Integration][C#] Add C Data Interface integration testing for C#~~ GH-37906: [Integration][C#] Add C Data Interface integration testing for C# Sep 27, 2023

pitrou changed the title ~~GH-37906: [Integration][C#] Add C Data Interface integration testing for C#~~ GH-37906: [Integration][C#] Implement C Data Interface integration testing for C# Sep 27, 2023

pitrou force-pushed the csharp-cdata-integration branch from f0b7413 to 4f1653a Compare September 27, 2023 14:10

pitrou marked this pull request as ready for review September 27, 2023 14:10

pitrou requested review from assignUser, kou, raulcd and westonpace as code owners September 27, 2023 14:10

pitrou commented Sep 27, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 27, 2023

pitrou commented Sep 27, 2023

View reviewed changes

pitrou requested review from eerhardt and removed request for kou, raulcd and assignUser September 27, 2023 14:42

wjones127 approved these changes Sep 27, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Sep 27, 2023

CurtHagenlocher reviewed Sep 27, 2023

View reviewed changes

csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs Outdated Show resolved Hide resolved

eerhardt approved these changes Oct 2, 2023

View reviewed changes

csharp/test/Apache.Arrow.IntegrationTest/CDataInterface.cs Outdated Show resolved Hide resolved

csharp/test/Apache.Arrow.IntegrationTest/CDataInterface.cs Outdated Show resolved Hide resolved

pitrou added 2 commits October 2, 2023 17:13

apacheGH-37906: [Integration][C#] Implement C Data Interface integrat…

025f1e3

…ion testing for C#

Failed attempt at making memory measurement more reliable

cc6f3e3

pitrou added 3 commits October 2, 2023 17:13

Style: brace placement

d1ab340

s/String/string/

5d31d5d

Remove unused function

d70236a

pitrou force-pushed the csharp-cdata-integration branch from 78c5c4f to d70236a Compare October 2, 2023 15:22

pitrou merged commit 7667b81 into apache:main Oct 2, 2023
15 checks passed

pitrou removed the awaiting merge Awaiting merge label Oct 2, 2023

pitrou deleted the csharp-cdata-integration branch October 2, 2023 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37906: [Integration][C#] Implement C Data Interface integration testing for C# #37904

GH-37906: [Integration][C#] Implement C Data Interface integration testing for C# #37904

pitrou commented Sep 27, 2023 •

edited by github-actions bot

Loading

This comment was marked as resolved.

pitrou commented Sep 27, 2023

pitrou Sep 27, 2023 •

edited

Loading

pitrou Sep 27, 2023

CurtHagenlocher Sep 27, 2023

CurtHagenlocher Sep 27, 2023

pitrou Sep 27, 2023

pitrou Sep 27, 2023

CurtHagenlocher Oct 2, 2023 •

edited

Loading

pitrou Oct 2, 2023

wjones127 left a comment

pitrou commented Sep 27, 2023

CurtHagenlocher commented Sep 27, 2023

pitrou commented Oct 2, 2023

conbench-apache-arrow bot commented Oct 2, 2023

GH-37906: [Integration][C#] Implement C Data Interface integration testing for C# #37904

GH-37906: [Integration][C#] Implement C Data Interface integration testing for C# #37904

Conversation

pitrou commented Sep 27, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

This comment was marked as resolved.

pitrou commented Sep 27, 2023

pitrou Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

pitrou Sep 27, 2023

Choose a reason for hiding this comment

CurtHagenlocher Sep 27, 2023

Choose a reason for hiding this comment

CurtHagenlocher Sep 27, 2023

Choose a reason for hiding this comment

pitrou Sep 27, 2023

Choose a reason for hiding this comment

pitrou Sep 27, 2023

Choose a reason for hiding this comment

CurtHagenlocher Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

pitrou Oct 2, 2023

Choose a reason for hiding this comment

wjones127 left a comment

Choose a reason for hiding this comment

pitrou commented Sep 27, 2023

CurtHagenlocher commented Sep 27, 2023

pitrou commented Oct 2, 2023

conbench-apache-arrow bot commented Oct 2, 2023

pitrou commented Sep 27, 2023 •

edited by github-actions bot

Loading

pitrou Sep 27, 2023 •

edited

Loading

CurtHagenlocher Oct 2, 2023 •

edited

Loading