ARROW-8883: [Rust] [Integration] Enable more tests

[nevi-me]

This enables more integration tests, and makes the necessary changes to make them pass.

---

Status

Primitive Arrays

These are passing, but I have questions on whether the way I'm comparing them is fine.
Specifically, when reading an array that has 3 values, the Buffer is returned aligned to 8 bytes, such that an i8's buffer would have { data: [v1, v2, v3, 0, 0, 0, 0, 0] }.

This causes problems, because when we reconstruct the above from JSON, we would have { data: [v1, v2, v3] }.

I have implemented PartialEq manually for ArrayData at 261c021. I'd appreciate some feedback on whether the approach is fine or not. I deliberately used assert_eq!(a, b, "helpful message") to make it a bit easier to track down the differences between objects being compared, as it's otherwise very difficult and tedious without some log!() implementation.

@jhorstmann 's work on bit slicing has been instrumental in allowing us to compare ArrayData, so I'm leveraging that for buffer comparisons. We however can end up with a situation where slicing buffers with offsets could lead to 2 buffers having different capacities. I don't think this is a bug.

Due to the above, I've also had to compare Buffer by length, and not capacity at 69f4ef3. @sunchao @jorgecarleitao PTAL and advise. I just made the change so tests could pass, but perhaps we could agree on what constitutes equal buffers (whether we should compare capacity).

I'd prefer to make the above change as a separate PR, so I can address offsets and also add tests for coverage.

Complex Types

Lists and structs currently fail because we can't carry custom names and nullability. For example, if a list's child is a list, such child could be named anything (I've seen item or list). We currently have no way of encoding this information; and thus tests fail on schema equality.

I opened https://issues.apache.org/jira/browse/ARROW-10261 to address this. I'll open a PR on the branch that's soon to be renamed from master 🤭, as this will affect Parquet and DataFusion.

Dictionaries

We seem to have covered a bit of dictonary support, but because we can't write dictionaries yet, we can't complete the integration testing.
I've opened https://issues.apache.org/jira/browse/ARROW-10268 for any interesting takers. I think it should be fairly trivial to implement, but I'm already trying to do too much.
@carols10cents this builds on the work that you did for dictionary support from arrow::ipc::conversion, so thanks for that 😃

Metadata

We fail on some metadata tests because arrow::datatypes::Field doesn't support metadata.
This one's a bit difficult to implement, as HashMap<String, String> doesn't support #[derive(Hash)] and other requirements.
I was thinking that maybe we could use Vec<(String, String)> and deal with deduplication of fields, and compatible JSON serde internally.

I opened https://issues.apache.org/jira/browse/ARROW-10259 for the above.

Quicker Testing

One of the reasons why I've taken long to work on this, was because I struggled a bit with running the integration tests (at least I had to go into a few dirs and build stuff). docker-compose --file docker-compose.yml run --rm conda-integration took long, and I was getting sporadic failures in Rust.

I've temporarily created a script called integration-testing.sh, so that whoever pitches in to help out, can use that to quickly build the C++, Java and Rust binaries for testing.

@pitrou @nealrichardson I'll remove the script and the .dockerignore entry before merging this.

Lastly, Antoine, seeing that we can't complete this in time for 2.0.0, I'm fine with leaving the implementation status fields on Rust as blank for a few more month.

Apologies for the "CC the world" update, but I think this is important enough for us to get more eyes on it.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-8883

[pitrou]

Remember to update the implementation status when done ;-)

[jorgecarleitao]

Thanks a lot for driving this and CCing. This is definitely important.

I have myself hit that capacity problem multiple times! One was when I was trying to simplify the equal.rs, for what you wrote on which buffers are different when the capacity is different, the second one was on #8401 , since when we have a buffer that receives data via the c data interface, we do not even know (or care) about its capacity.

AFAI can tell, we have two use-cases of capacity atm:

• deallocating the region when it is no longer used
• computing the total size in bytes of arrays

Because arrays share buffers, the total size of an array is currently misleading. For example, when the array is computed from is_not_null of another array, both the null buffer (buffer 0) and the value (buffer 1) share the same memory region, and thus IMO the total size computation based on capacity is incorrect. This is also true for complex structs on which buffers are shared within the same array.

Thus, IMO capacity main use-case is for bookkeeping, on how to de-allocate the region. #8401 systematizes that idea, on which BufferData (renamed Bytes there) no longer has capacity, but an enum about how to de-allocate itself.

Regardless, I would say that the vast majority of the use-cases on which we want to compare buffers is when we want to compare its contents, irrespectively of how they should be deallocated / capacity. Therefore, I would be happy to have buffer comparison be based on their actual content (in bytes). We just need to be careful about bitmaps, on which the comparison should be made in bits.

[nevi-me]

Thus, IMO capacity main use-case is for bookkeeping, on how to de-allocate the region. #8401 systematizes that idea, on which BufferData (renamed Bytes there) no longer has capacity, but an enum about how to de-allocate itself.

I agree with your thinking here, especially on equality and arrays with offsets. One of the things we don't test (extensively or at all) is whether we're able to write arrays with unaligned offsets to the IPC format. I'll put in time to work on this whenever I get to v5 of the Arrow format.

Regardless, I would say that the vast majority of the use-cases on which we want to compare buffers is when we want to compare its contents, irrespectively of how they should be deallocated / capacity. Therefore, I would be happy to have buffer comparison be based on their actual content (in bytes). We just need to be careful about bitmaps, on which the comparison should be made in bits.

I'd also prefer not to compare using buffer capacity, as I'm mainly interested in the buffer contents, especially when dealing with a frozen buffer where we won't be modifying contents.

On bitmaps, I'll have a look, but I think that the approach of using the bit iterators should cover this.

Apologies again for not having gotten to #8401 yet, I hope you're not in a rush to get it implemented (esp as we're now looking at 3.0 for all new work).

[jorgecarleitao]

I did a quick run trough and left some comments. I am still missing the large bin file.

So far, all changes make sense to me (apart from the comments).

[nevi-me]

@jorgecarleitao @carols10cents I'm contemplating closing this, and breaking it up into smaller PRs (if this round of changes don't pass integration). I'll work on it this week in my evenings (GMT+2) so I can get it out of the way, and go back to working on Parquet.

[jorgecarleitao]

From what I have read in this PR, it looks good so far. Is there anything blocking it?

[nevi-me]

From what I have read in this PR, it looks good so far. Is there anything blocking it?

@jorgecarleitao the actual integration tests not passing, are the blocker 😅

There were 10 failures in the last run, logical equality related. It's actually odd, because on my local testing I get 120+ failures between Rust and the other langs (except C++, that passes).

I'll monitor https://github.com/apache/arrow/pull/8200/checks?check_run_id=1378263093 in the background, if it passes; we can merge this (if you don't have any outstanding queries/concerns).

I used your suggested implementation of logical equality (dde96c6)

[nevi-me]

@jorgecarleitao we're down to 3 failures.

thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 0', arrow/src/array/equal/variable_size.rs:31:21

Looks like we're not comparing zero length arrays correctly

[nevi-me]

@jorgecarleitao finally passed! There's still some tests that don't pass, but I've updated the status documenation with them (cc @andygrove as you had opened a JIRA for this)