[C++] ValidateArray is out of sync with the ListArray IPC specification

[asfimport]

• It appears to check that null values take zero space
• It still checks for a begin offset of 0 if the array isn't sliced (technically this doesn't seem necessary and it could be non-zero even if the array wasn't sliced.)
• I think it also fails if an array is sliced to truncate it since it should compare length to data_extent instead of last_offset.

Reporter: Micah Kornfield / @emkornfield
Assignee: Ben Kietzman / @bkietz

PRs and other links:

• GitHub Pull Request #6269

_{Note: This issue was originally created as ARROW-6929. Please see the migration documentation for further details.}

[asfimport]

Antoine Pitrou / @pitrou:
[~emkornfield@gmail.com] Can you check the current implementation? It's been significantly overhauled.

[asfimport]

Antoine Pitrou / @pitrou:
Ping [~emkornfield@gmail.com]. If you could give a check to the current implementation.

[asfimport]

Micah Kornfield / @emkornfield:
apologies, I'll check by end of week.

[asfimport]

Micah Kornfield / @emkornfield:
I'm not sure the condition checked ( "if (array.length() > 0 && array.offset() == 0 && array.value_offset(0) != 0)")
return Status::Invalid("The first offset isn't zero");
} at https://github.com/apache/arrow/blob/master/cpp/src/arrow/array/validate.cc#L249 is a requirement for arrays ?

[asfimport]

Ben Kietzman / @bkietz:
[~emkornfield@gmail.com] The "first offset == 0" requirement was explicitly stated in Layout.rst, line 286. It was removed in 67d46c7 but that may not have been intentional, @wesm? I agree that requirement seems unnecessary and could be safely removed now.

I believe the other two points have been resolved: validate no longer checks for empty lists "under" null bits and I don't observe incorrect logic in the case of ValidateListArray(trucated).

[asfimport]

Micah Kornfield / @emkornfield:
agreed the other two have been resolved.

[asfimport]

Wes McKinney / @wesm:
Reviewing the changes, I agree that the first offset is not required to be zero, but it is a guideline (and note we shift everything to 0 in the C++ library on IPC write). If the first offset is non-zero then you are sending more data than you need to, so I think this validation check can be removed

[asfimport]

Neal Richardson / @nealrichardson:
@wesm [~emkornfield@gmail.com] @bkietz So this is done, right?

[asfimport]

Ben Kietzman / @bkietz:
@nealrichardson Almost: removing that unnecessary check in #6269

[asfimport]

Micah Kornfield / @emkornfield:
Issue resolved by pull request 6269
#6269