GH-41771: [C++] Iterator releases its resource immediately when it reads all values

[kou]

Rationale for this change

Iterator keeps its resource (ptr_) until it's deleted but we can release its resource immediately when it reads all values. If Iterator keeps its resource until it's deleted, it may block closing a file. See GH-41771 for this case.

What changes are included in this PR?

Releases ptr_ when Next() returns the end.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

• GitHub Issue: [C++] Consuming or closing a RecordBatchReader created from a Dataset Scanner does not close underlying files #41771

[github-actions]

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

[bkietz]

LGTM

[felipecrv]

@kou @bkietz This extra check and release should be the responsibility of the underlying iterator instead of forcing every abstract iterator to behave this way.

[kou]

Could you explain the reason? Performance?

For the #41771 case:

This is the underlying iterator:

arrow/cpp/src/arrow/dataset/scanner.cc

Lines 380 to 386 in 7f0c407

	Result<TaggedRecordBatchIterator> AsyncScanner::ScanBatches() {
	return ::arrow::internal::IterateSynchronously<TaggedRecordBatch>(
	[this](::arrow::internal::Executor* executor) {
	return ScanBatchesAsync(executor);
	},
	scan_options_->use_threads);
	}

We want to release the IPC reader created at:

arrow/cpp/src/arrow/dataset/file_ipc.cc

Line 144 in 7bc2452

auto open_reader = OpenReaderAsync(source);

It's referred by the underlying iterator indirectly via Future/std::function.

It seems that we can't remove a reference from Future/std::function without deleting Future/std::function and we can't delete Future/std::function without deleting the underlying iterator.

[felipecrv]

Could you explain the reason? Performance?

Performance, binary code size, and overall elegance of the iterator tree.

But I see that it's not possible because these iterators are like C++ in that the iterator itself is the value as well instead of being something that produces the value. So just ignore my comment.

[conbench-apache-arrow]

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

There were 6 benchmark results indicating a performance regression:

• Commit Run on ec2-m5-4xlarge-us-east-2 at 2024-05-28 15:42:38Z
  • dataset-serialize (Python) with dataset=nyctaxi_multi_parquet_s3, format=csv, selectivity=1pc
  • dataset-serialize (Python) with dataset=nyctaxi_multi_ipc_s3, format=csv, selectivity=1pc
• and 4 more (see the report linked below)

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