GH-32439: [Python] Fix off by one bug when chunking nested structs

[mikelui]

Rationale for this change

See: #32439

What changes are included in this PR?

During conversion from Python to Arrow, when a struct's child hits a capacity error and chunking is triggered, this can leave the Finish'd chunk in an invalid state since the struct's length does not match the length of its children.

This change simply tries to Append the children first, and only if successful will Append the struct. This is safe because the order of Append'ing between the struct and its child is not specified. It is only specified that they must be consistent with each other.

This is per:

arrow/cpp/src/arrow/array/builder_nested.h

Lines 507 to 508 in 86b7a84

	/// Append an element to the Struct. All child-builders' Append method must
	/// be called independently to maintain data-structure consistency.

Are these changes tested?

A unit test is added that would previously have an invalid data error.

>       tab = pa.Table.from_pandas(df)

pyarrow/tests/test_pandas.py:4970: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
pyarrow/table.pxi:3788: in pyarrow.lib.Table.from_pandas
    return cls.from_arrays(arrays, schema=schema)
pyarrow/table.pxi:3890: in pyarrow.lib.Table.from_arrays
    result.validate()
pyarrow/table.pxi:3170: in pyarrow.lib.Table.validate
    check_status(self.table.Validate())

# ...

FAILED pyarrow/tests/test_pandas.py::test_nested_chunking_valid - pyarrow.lib.ArrowInvalid: Column 0: In chunk 0: Invalid: List child array invalid: Invalid: Struct child array #0 has length smaller than expected for struct array (2 < 3)

NOTE: This unit test uses about 7GB of memory (max RSS) on my macbook pro. This might make CI challenging; I'm open to suggestions to limit it.

Are there any user-facing changes?

No

• Closes: [Python] Converting data frame to Table with large nested column fails Invalid Struct child array has length smaller than expected #32439

[github-actions]

⚠️ GitHub issue #32439 has been automatically assigned in GitHub to PR creator.

[mikelui]

Failures are due to high memory from the unit test 😮‍💨

[mikelui]

Lowered the size of the binary array to be within mem limits (barely scraping by 🥹)

720,000,000 * 3 = 2,160,000,000, which triggers chunking over 2,147,483,647

---

edit: aaarg Python 3.8 w/ Pandas 1.0 works, while Python 3.10 with Pandas latest hits mem limits.

This test is specifically for cases with high memory and chunking requirements. I'm open to other ideas for testing

[mikelui]

I removed the additional test that was blocking tests, due to intensive memory requirements.

For posterity, I left the test in as the initial commit (with the removal being a successive commit). Folks can confirm the behavior fix independently.

I don't think we should block fixing a critical bug on the memory intensive test.

---

EDIT: Realized I can mark tests as large memory.
Ran all the large memory tests locally and found that I needed to set PyStructConverter to not rewind on a capacity error, since now we wait for confirmation that child builders succeeded before appending there.

The test that exposed this is:

arrow/python/pyarrow/tests/test_convert_builtin.py

Lines 2293 to 2319 in c9bfca5

	@pytest.mark.large_memory
	@pytest.mark.parametrize(('ty', 'char'), [
	(pa.string(), 'x'),
	(pa.binary(), b'x'),
	])
	def test_nested_auto_chunking(ty, char):
	v1 = char * 100000000
	v2 = char * 147483646
	struct_type = pa.struct([
	pa.field('bool', pa.bool_()),
	pa.field('integer', pa.int64()),
	pa.field('string-like', ty),
	])
	data = [{'bool': True, 'integer': 1, 'string-like': v1}] * 20
	data.append({'bool': True, 'integer': 1, 'string-like': v2})
	arr = pa.array(data, type=struct_type)
	assert isinstance(arr, pa.Array)
	data.append({'bool': True, 'integer': 1, 'string-like': char})
	arr = pa.array(data, type=struct_type)
	assert isinstance(arr, pa.ChunkedArray)
	assert arr.num_chunks == 2
	assert len(arr.chunk(0)) == 21
	assert len(arr.chunk(1)) == 1
	assert arr.chunk(1)[0].as_py() == {

[c0g]

I’m hitting this error too, any chance this PR could be merged?

[mikelui]

@westonpace since you seemed to be PoC on the GH issue, can you take a look or direct us to the right person to review this? It seems there's a bit of a backlog in reviewing [Python] PR's, anyway.

[mikelui]

@AlenkaF @wjones127 There are some backlog PRs is there any chance of getting this reviewed?

[AlenkaF]

Sorry for slow response from us @mikelui , thank you for keeping it going!
I plan to look at the PR today.

PS: The mark for the tests with large memory is the correct way to go 👍

[AlenkaF]

LGTM +1

The solution is elegant IMO. The tests added are also giving good coverage. I would only have another sanity check of the C++ change from somebody else and then I am happy to merge!

[mikelui]

cc @westonpace @wjones127 @mapleFU @pitrou can someone take a look here? 🥹

[pitrou]

I pushed small test improvements and rebased from latest git main. Thanks for this @mikelui !

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 8cdce28.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.