Giorgi / DuckDB.NET

Bindings and ADO.NET Provider for DuckDB
https://duckdb.net
MIT License
338 stars 61 forks source link

Fix Proposal for Issue 199 #200

Closed brendonparker closed 2 weeks ago

brendonparker commented 2 weeks ago

Fixes https://github.com/Giorgi/DuckDB.NET/issues/199

Maybe a more elegant way?

Not sure the test I added fits your standards, but gives some idea of what is going on.

codecov-commenter commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes missing coverage. Please review.

Project coverage is 86.92%. Comparing base (e2e2e71) to head (9e62861).

Files Patch % Lines
...B.NET.Data/Internal/Writer/ListVectorDataWriter.cs 4.76% 20 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #200 +/- ## =========================================== - Coverage 88.63% 86.92% -1.71% =========================================== Files 56 56 Lines 1830 1851 +21 Branches 252 252 =========================================== - Hits 1622 1609 -13 - Misses 150 184 +34 Partials 58 58 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9571608010

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
DuckDB.NET.Data/Internal/Writer/ListVectorDataWriter.cs 1 21 4.76%
<!-- Total: 1 21 4.76% -->
Files with Coverage Reduction New Missed Lines %
DuckDB.NET.Data/Internal/Writer/ListVectorDataWriter.cs 2 74.51%
<!-- Total: 2 -->
Totals Coverage Status
Change from base Build 9561880599: -1.2%
Covered Lines: 1681
Relevant Lines: 1851

💛 - Coveralls
Giorgi commented 2 weeks ago

The code is exactly what I was planning to do. For tests, I wanted to suggest this but it's failing even with the fix. Not sure why.

    [Fact]
    public void ListValuesDoubleNullable()
    {
        ListValuesInternal("Double", faker => faker.Random.Double().OrNull(faker));
    }
brendonparker commented 2 weeks ago

Thanks for pointing me towards those tests. I took a quick look but it isn't apparent to me either :/

I think it may be memory related? At least on my machine the test either fails with:

The active test run was aborted. Reason: Test host process crashed

or

The active test run was aborted. Reason: Test host process crashed : Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

If I change the rows from 2000 to 10 then it works/passes 🤔

Giorgi commented 2 weeks ago

I have noticed that the test runners report a wrong message when Access Violation or some other low-level error causes the failure. You'll actually need to debug the test to see what error is thrown and where. I'm also not sure why changing the row number helps. Probably it's generating some edge case data when the number of rows is higher.

brendonparker commented 2 weeks ago

Agree... not a change you want to leave/keep. I just added it to illustrate.

I need to take a break for a while. I may poke at it a bit more later to try and figure out what is going wrong.

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9572221321

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
DuckDB.NET.Data/Internal/Writer/ListVectorDataWriter.cs 1 21 4.76%
<!-- Total: 1 21 4.76% -->
Files with Coverage Reduction New Missed Lines %
DuckDB.NET.Data/Internal/Writer/VectorDataWriterBase.cs 2 79.82%
DuckDB.NET.Data/Internal/Writer/ListVectorDataWriter.cs 14 62.75%
<!-- Total: 16 -->
Totals Coverage Status
Change from base Build 9561880599: -2.1%
Covered Lines: 1667
Relevant Lines: 1851

💛 - Coveralls
Giorgi commented 2 weeks ago

Found the issue!