Skip to content

[SPARK-52516][SQL] Don't hold previous iterator reference after advancing to next file in ParquetPartitionReaderFactory - #51528

Closed
viirya wants to merge 2 commits into
apache:masterfrom
viirya:fix_iter_callback
Closed

[SPARK-52516][SQL] Don't hold previous iterator reference after advancing to next file in ParquetPartitionReaderFactory#51528
viirya wants to merge 2 commits into
apache:masterfrom
viirya:fix_iter_callback

Conversation

@viirya

@viiryaviirya commented Jul 17, 2025

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This patch adds only one task completion listener for closing iterators in ParquetPartitionReaderFactory, instead of adding separate one for each file iterator.

Why are the changes needed?

For each file iterator, currently we add one task completion listener used to update closing the iterator when the task is finished. In the listener, the iterator reference is held. So if the file is normally exhausted, the reference cannot be released early. It is a problem especially if the reference are heavy as reported by apache/iceberg#13297.

Similar to #51503, we don't need set up a listener for each file iterator. Thus, we can set up just one listener for all iterator. Once we advance to next file, we can update the update target to new iterator.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests.

Was this patch authored or co-authored using generative AI tooling?

No

@viiryaviirya changed the title [SPARK-XXXXX][SQL] Don't hold previous iterator reference after advancing to next file in ParquetPartitionReaderFactory[SPARK-52516][SQL] Don't hold previous iterator reference after advancing to next file in ParquetPartitionReaderFactoryJul 17, 2025
dongjoon-hyun
dongjoon-hyun reviewed Jul 17, 2025
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this causes many Parquet-related test case failures somehow. Could you double-check the test cases, @viirya ?

@dongjoon-hyun

dongjoon-hyun commented Jul 17, 2025

Copy link
Copy Markdown
Member

cc @peter-toth

@viirya

viirya commented Jul 17, 2025

Copy link
Copy Markdown
MemberAuthor

Sure, let me check it.

@viirya
viiryaforce-pushed the fix_iter_callback branch from 7f2023d to d39dd1bCompareJuly 17, 2025 15:53
@viirya

viirya commented Jul 17, 2025

Copy link
Copy Markdown
MemberAuthor

@dongjoon-hyun It should be fixed now. Thanks.

@viiryaviirya mentioned this pull request Jul 17, 2025
Closed
3 tasks
dongjoon-hyun
dongjoon-hyun approved these changes Jul 17, 2025
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM.

szehon-ho
szehon-ho approved these changes Jul 17, 2025
View reviewed changes
Comment thread...la/org/apache/spark/sql/execution/datasources/v2/parquet/ParquetPartitionReaderFactory.scala Outdated
this.iter = iter
}

def execute(): Unit = {

@szehon-hoszehon-hoJul 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: closeCurrent()?

@viiryaviiryaJul 18, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay

@viirya

viirya commented Jul 18, 2025

Copy link
Copy Markdown
MemberAuthor

All tests are passed: https://github.com/viirya/spark-1/runs/46226372373

viirya added a commit that referenced this pull request Jul 18, 2025
@viirya
[SPARK-52516][SQL] Don't hold previous iterator reference after advan…
97adac6
…cing to next file in ParquetPartitionReaderFactory
### What changes were proposed in this pull request?
This patch adds only one task completion listener for closing iterators in `ParquetPartitionReaderFactory`, instead of adding separate one for each file iterator.
### Why are the changes needed?
For each file iterator, currently we add one task completion listener used to update closing the iterator when the task is finished. In the listener, the iterator reference is held. So if the file is normally exhausted, the reference cannot be released early. It is a problem especially if the reference are heavy as reported by apache/iceberg#13297.
Similar to #51503, we don't need set up a listener for each file iterator. Thus, we can set up just one listener for all iterator. Once we advance to next file, we can update the update target to new iterator.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing tests.
### Was this patch authored or co-authored using generative AI tooling?
No
Closes#51528 from viirya/fix_iter_callback.
Authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>
(cherry picked from commit 197c9d6)
Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>
viirya added a commit that referenced this pull request Jul 18, 2025
@viirya
[SPARK-52516][SQL] Don't hold previous iterator reference after advan…
ebe6ca8
…cing to next file in ParquetPartitionReaderFactory
### What changes were proposed in this pull request?
This patch adds only one task completion listener for closing iterators in `ParquetPartitionReaderFactory`, instead of adding separate one for each file iterator.
### Why are the changes needed?
For each file iterator, currently we add one task completion listener used to update closing the iterator when the task is finished. In the listener, the iterator reference is held. So if the file is normally exhausted, the reference cannot be released early. It is a problem especially if the reference are heavy as reported by apache/iceberg#13297.
Similar to #51503, we don't need set up a listener for each file iterator. Thus, we can set up just one listener for all iterator. Once we advance to next file, we can update the update target to new iterator.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing tests.
### Was this patch authored or co-authored using generative AI tooling?
No
Closes#51528 from viirya/fix_iter_callback.
Authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>
(cherry picked from commit 197c9d6)
Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>
@viirya
viirya deleted the fix_iter_callback branch July 18, 2025 08:02
@viirya

viirya commented Jul 18, 2025

Copy link
Copy Markdown
MemberAuthor

Thanks @dongjoon-hyun@peter-toth@szehon-ho

@dongjoon-hyun

dongjoon-hyun commented Jul 18, 2025

Copy link
Copy Markdown
Member

Thank you.

Could you update the Fix Versions and Assignee fields too, @viirya ?

Screenshot 2025-07-18 at 07 46 11

@viirya

viirya commented Jul 18, 2025

Copy link
Copy Markdown
MemberAuthor

Thank you @dongjoon-hyun. I just fixed it.

zifeif2 pushed a commit to zifeif2/spark that referenced this pull request Nov 14, 2025
@viirya
[SPARK-52516][SQL] Don't hold previous iterator reference after advan…
0b36549
…cing to next file in ParquetPartitionReaderFactory
### What changes were proposed in this pull request?
This patch adds only one task completion listener for closing iterators in `ParquetPartitionReaderFactory`, instead of adding separate one for each file iterator.
### Why are the changes needed?
For each file iterator, currently we add one task completion listener used to update closing the iterator when the task is finished. In the listener, the iterator reference is held. So if the file is normally exhausted, the reference cannot be released early. It is a problem especially if the reference are heavy as reported by apache/iceberg#13297.
Similar to apache#51503, we don't need set up a listener for each file iterator. Thus, we can set up just one listener for all iterator. Once we advance to next file, we can update the update target to new iterator.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing tests.
### Was this patch authored or co-authored using generative AI tooling?
No
Closesapache#51528 from viirya/fix_iter_callback.
Authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>
(cherry picked from commit dcdb482)
Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>
Asmoday pushed a commit to arenadata/spark that referenced this pull request Feb 20, 2026
@viirya@Asmoday
[SPARK-52516][SQL] Don't hold previous iterator reference after advan…
51aa24b
…cing to next file in ParquetPartitionReaderFactory
### What changes were proposed in this pull request?
This patch adds only one task completion listener for closing iterators in `ParquetPartitionReaderFactory`, instead of adding separate one for each file iterator.
### Why are the changes needed?
For each file iterator, currently we add one task completion listener used to update closing the iterator when the task is finished. In the listener, the iterator reference is held. So if the file is normally exhausted, the reference cannot be released early. It is a problem especially if the reference are heavy as reported by apache/iceberg#13297.
Similar to apache#51503, we don't need set up a listener for each file iterator. Thus, we can set up just one listener for all iterator. Once we advance to next file, we can update the update target to new iterator.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing tests.
### Was this patch authored or co-authored using generative AI tooling?
No
Closesapache#51528 from viirya/fix_iter_callback.
Authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>
(cherry picked from commit 197c9d6)
Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peter-tothpeter-tothpeter-toth approved these changes
@dongjoon-hyundongjoon-hyundongjoon-hyun approved these changes
@szehon-hoszehon-hoszehon-ho approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@viirya@dongjoon-hyun@peter-toth@szehon-ho