Skip to content

GH-47657: [C++][Parquet] Check for integer overflow when coercing timestamps#49615

Merged
wgtmac merged 4 commits into
apache:mainfrom
ArnavBalyan:arnavb/ts-overflow
Apr 29, 2026
Merged

GH-47657: [C++][Parquet] Check for integer overflow when coercing timestamps#49615
wgtmac merged 4 commits into
apache:mainfrom
ArnavBalyan:arnavb/ts-overflow

Conversation

@ArnavBalyan

@ArnavBalyan ArnavBalyan commented Mar 29, 2026 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

Rationale for this change

  • When writing Arrow timestamps to Parquet, timestamp values are multiplied to convert units
  • This multiplication was performed without overflow checking which can silently write corrupt data.

What changes are included in this PR?

  • Use MultiplyWithOverflowGeneric to handle/detect overflow

Are these changes tested?

  • Yes

Are there any user-facing changes?

  • Yes

@ArnavBalyan ArnavBalyan requested a review from wgtmac as a code owner March 29, 2026 05:46
@github-actions

github-actions Bot commented Mar 29, 2026

Copy link
Copy Markdown

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

wgtmac
wgtmac reviewed Mar 30, 2026
View reviewed changes

@wgtmac wgtmac 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.

Thanks for fixing this! This fix generally looks good. I've left some inline comments.

Comment thread cpp/src/parquet/column_writer.cc Outdated
Comment thread cpp/src/parquet/arrow/arrow_reader_writer_test.cc Outdated
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Mar 30, 2026
wgtmac
wgtmac reviewed Mar 31, 2026
View reviewed changes

@wgtmac wgtmac 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.

Some nits :)

Comment thread cpp/src/parquet/arrow/arrow_reader_writer_test.cc Outdated
Comment thread cpp/src/parquet/arrow/arrow_reader_writer_test.cc Outdated
Comment thread cpp/src/parquet/arrow/arrow_reader_writer_test.cc
wgtmac
wgtmac approved these changes Apr 2, 2026
View reviewed changes

@wgtmac wgtmac 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

Thanks a lot, @ArnavBalyan!

@ArnavBalyan

ArnavBalyan commented Apr 25, 2026

Copy link
Copy Markdown
Member Author

Thanks @wgtmac! Could this be merged

@wgtmac wgtmac merged commit b3dcb6d into apache:main Apr 29, 2026
51 of 52 checks passed
@wgtmac wgtmac removed the awaiting committer review Awaiting committer review label Apr 29, 2026
@wgtmac wgtmac mentioned this pull request Apr 29, 2026
Closed
@wgtmac

wgtmac commented Apr 29, 2026

Copy link
Copy Markdown
Member

Merged. Thanks @ArnavBalyan!

@ArnavBalyan

ArnavBalyan commented Apr 29, 2026

Copy link
Copy Markdown
Member Author

Thanks @wgtmac!

@conbench-apache-arrow

conbench-apache-arrow Bot commented Apr 29, 2026

Copy link
Copy Markdown

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

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

@conbench-apache-arrow

conbench-apache-arrow Bot commented May 14, 2026

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Mottl pushed a commit to Mottl/arrow that referenced this pull request May 26, 2026
@ArnavBalyan @Mottl
apacheGH-47657: [C++][Parquet] Check for integer overflow when coerci…
42a3087 
…ng timestamps (apache#49615)

### Rationale for this change
 - When writing Arrow timestamps to Parquet, timestamp values are multiplied to convert units
 - This multiplication was performed without overflow checking which can silently write corrupt data.

### What changes are included in this PR?
 - Use MultiplyWithOverflowGeneric to handle/detect overflow

### Are these changes tested?
 - Yes
### Are there any user-facing changes?
 - Yes

* GitHub Issue: apache#47657

Authored-by: Arnav Balyan <arnavbalyan1@gmail.com>
Signed-off-by: Gang Wu <ustcwg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wgtmac wgtmac wgtmac approved these changes

Assignees

No one assigned

Labels

Component: C++ Component: Parquet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ArnavBalyan @wgtmac