Added raw_values to SliceMut

[jorgecarleitao]

No description provided.

[coveralls]

Pull Request Test Coverage Report for Build 8e5bd2648a9b25b8eaaa768f6936a8f9ebfb3947-PR-174

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 84.153%

---

Totals
Change from base Build e8ab681af2c8950c9baa65b406ead0e49df86ae6:	0.01%
Covered Lines:	3643
Relevant Lines:	4329

---

💛 - Coveralls

[pacman82]

F**k. I thought I merged this slightly different branch weeks ago: f8b2b23 !

Sorry, I'll take care of it this evening. Seems also like I duplicated some work here.

[pacman82]

I even added it to the Changelog of 0.35.1. I clearly do not get enough sleep lately 😓

[pacman82]

Wait, seems I did merge it

[pacman82]

Feature is already deployed: https://docs.rs/odbc-api/latest/odbc_api/buffers/struct.NullableSlice.html#method.raw_values

or do I miss something?

[pacman82]

Ahh, now I get it, its about mutable slices!

[pacman82]

That method must be unsafe though, because not every driver (looking at you MariaDB) does checks wether the indicator value is larger than the column. It's easy then to create access to invalid memory in safe code, by specifying an indicator too large.

See:

odbc-api/odbc-api/tests/integration.rs

Line 2306 in e8ab681

let memory = "Hello\0INVALID MEMORY\0";

There is a soundness issue I still need to close around ColumnBuffer for these drivers. Issue is different invariants for Buffers regarding Reading and Writing.

[jorgecarleitao]

uhm that is a good point. Maybe a function that consumes an iterator of bools and populates the indicators accordingly? This would avoid overflowing the buffer's max len?

[pacman82]

Maybe a function that consumes an iterator of bools and populates the indicators

Possible, but didn't you want to access it in parallel.

That method must be unsafe though

I may walk back on that. Come to think of it, this is only invoked on fixed sized (POD) types. The issue I raised would only apply to text / binary columns. According to the standard ODBC should only care about wether this is NULL data or not. Some drivers might still use indicators to calculate the offset, but would be willing to deem this method safe until proven otherwise.

[jorgecarleitao]

I left the binary out until we check whether we need to expose that at all (based on benches).

[pacman82]

It still fails with the docs

[pacman82]

Hm, come to think of it, only the binary layout of the value buffer is identical. I'd prefer to go with your idea of taking an iterator over booleans for the indicator. Would this allow you to write code just as fast?

@jorgecarleitao

[jorgecarleitao]

would a patch with this be possible? It would allow me to release arrow2 with the ODBC to crates.io ^^

[pacman82]

sure, just wondering about the interface to the indicator

[pacman82]

would exposing only the raw values, together with the possibility to take an iterator over bools allow for the same performance gains.

[pacman82]

I'll release now, with the current interface, and break it later in case your answer will turn out to be 'yes'.

[jorgecarleitao]

you mean for the binColumn?

[pacman82]

I meant actually for NullableSliceMut and NullableSlice. Since the indicator array is different from the bitmask of arrow anyhow.

[pacman82]

Version 0.35.2 is released. This should unblock us. I'll be able to invest more work in the future, but as I said I am preoccupied at the moment.