Skip to content

GH-38591: [Parquet][C++] Remove redundant open calls in ParquetFileFormat::GetReaderAsync - #38621

Merged
mapleFU merged 1 commit into
apache:mainfrom
eeroel:fix/parquet_remove_redundant_opens
Nov 8, 2023
Merged

GH-38591: [Parquet][C++] Remove redundant open calls in ParquetFileFormat::GetReaderAsync#38621
mapleFU merged 1 commit into
apache:mainfrom
eeroel:fix/parquet_remove_redundant_opens

Conversation

@eeroel

@eeroeleeroel commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

Rationale for this change

There were duplicate method calls causing extra I/O operations, apparently unintentional from 0793432.

What changes are included in this PR?

Remove the extra method calls.

Are these changes tested?

Are there any user-facing changes?

@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@eeroeleeroel changed the title [GH-38591]: [Parquet] Remove redundant open callsGH-38591: [Parquet] Remove redundant open callsNov 7, 2023
@github-actions

Copy link
Copy Markdown

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

@mapleFU

Copy link
Copy Markdown
Member

Will merge in one day if don't have any further comments

@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 7, 2023
@mapleFU

Copy link
Copy Markdown
Member

Also cc @raulcd would this better in 14.0.1?

@raulcd

Copy link
Copy Markdown
Member

Also cc @raulcd would this better in 14.0.1?

14.0.1 is frozen and ready to be shipped

@mapleFUmapleFU changed the title GH-38591: [Parquet] Remove redundant open callsGH-38591: [Parquet][C++] Remove redundant open calls in ParquetFileFormat::GetReaderAsyncNov 8, 2023
@mapleFU
mapleFU merged commit e62ec62 into apache:mainNov 8, 2023
@mapleFUmapleFU removed the awaiting committer review Awaiting committer review label Nov 8, 2023
@jorisvandenbossche

Copy link
Copy Markdown
Member

I labeled the issue as backport-candidate, though, in case we do another bug fix release

@mapleFU

Copy link
Copy Markdown
Member

@jorisvandenbossche Maybe this should also related to #38466 and #38437 ? I'll drive them to be merged this week

@conbench-apache-arrow

Copy link
Copy Markdown

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

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.

loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…tFileFormat::GetReaderAsync` (apache#38621)
### Rationale for this change
There were duplicate method calls causing extra I/O operations, apparently unintentional from apache@0793432.
### What changes are included in this PR?
Remove the extra method calls.
### Are these changes tested?
### Are there any user-facing changes?
* Closes: apache#38591
Authored-by: Eero Lihavainen <eero.lihavainen@nitor.com>
Signed-off-by: mwish <maplewish117@gmail.com>
raulcd pushed a commit that referenced this pull request Nov 28, 2023
…ormat::GetReaderAsync` (#38621)
### Rationale for this change
There were duplicate method calls causing extra I/O operations, apparently unintentional from 0793432.
### What changes are included in this PR?
Remove the extra method calls.
### Are these changes tested?
### Are there any user-facing changes?
* Closes: #38591
Authored-by: Eero Lihavainen <eero.lihavainen@nitor.com>
Signed-off-by: mwish <maplewish117@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…tFileFormat::GetReaderAsync` (apache#38621)
### Rationale for this change
There were duplicate method calls causing extra I/O operations, apparently unintentional from apache@0793432.
### What changes are included in this PR?
Remove the extra method calls.
### Are these changes tested?
### Are there any user-facing changes?
* Closes: apache#38591
Authored-by: Eero Lihavainen <eero.lihavainen@nitor.com>
Signed-off-by: mwish <maplewish117@gmail.com>
Sign up for freeto 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.

[Parquet][Python] Potential regression in Parquet parallel reading

4 participants

@eeroel@mapleFU@raulcd@jorisvandenbossche