Skip to content

Conversation

@HuaHuaY
Copy link
Contributor

@HuaHuaY HuaHuaY commented Sep 28, 2025

Rationale for this change

Fix wrong result of num_rows() method in FileSerializer.

What changes are included in this PR?

  1. add num_rows_ before each call to RowGroupWriter::Close in FileSerializer.

Are these changes tested?

Yes.

Are there any user-facing changes?

Now num_rows_ will return the corrent result which is the number of rows in the yet started RowGroups.

@HuaHuaY HuaHuaY requested a review from wgtmac as a code owner September 28, 2025 11:28
@github-actions
Copy link

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

@HuaHuaY
Copy link
Contributor Author

HuaHuaY commented Sep 28, 2025

@pitrou @wgtmac @mapleFU Please take a look.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 28, 2025
@pitrou pitrou merged commit 5750e29 into apache:main Oct 9, 2025
36 of 38 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Oct 9, 2025
@conbench-apache-arrow
Copy link

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

There was 1 benchmark result indicating a performance regression:

The full Conbench report has more details.

@HuaHuaY HuaHuaY deleted the fix_issue_47664 branch October 10, 2025 02:26
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this pull request Oct 15, 2025
…roupWriter::Close in FileSerializer (apache#47665)

### Rationale for this change

Fix wrong result of `num_rows()` method in `FileSerializer`.

### What changes are included in this PR?

1. add `num_rows_` before each call to `RowGroupWriter::Close` in `FileSerializer`.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Now `num_rows_` will return the corrent result which is the number of rows in the yet started RowGroups.

* GitHub Issue: apache#47664

Authored-by: Zehua Zou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants