Skip to content

GH-34807: [Go] Handle io.EOF when reading parquet footer size and magic bytes#34808

Merged
zeroshade merged 5 commits into
apache:mainfrom
agchang:agc/34807/parquet-eof
Apr 10, 2023
Merged

GH-34807: [Go] Handle io.EOF when reading parquet footer size and magic bytes#34808
zeroshade merged 5 commits into
apache:mainfrom
agchang:agc/34807/parquet-eof

Conversation

@agchang

@agchang agchang commented Mar 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Mar 30, 2023

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Mar 30, 2023

Copy link
Copy Markdown

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

@zeroshade

zeroshade commented Apr 4, 2023

Copy link
Copy Markdown
Member

Is there more you intend to do here? I'd say that a unit test which uses an implementation that returns the io.EOF and confirms this fixes it would be good. Otherwise this seems fine to me if you think it's ready for review.

@agchang agchang marked this pull request as ready for review April 5, 2023 00:05
@agchang agchang requested a review from zeroshade as a code owner April 5, 2023 00:05
@agchang

agchang commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

Is there more you intend to do here? I'd say that a unit test which uses an implementation that returns the io.EOF and confirms this fixes it would be good. Otherwise this seems fine to me if you think it's ready for review.

I added a test. I wasn't sure of the best way to get a buffer to an underlying valid parquet file for testing purposes, so I pieced together things from other tests -- let me know if there's a better way.

zeroshade
zeroshade reviewed Apr 5, 2023
View reviewed changes
Comment thread go/parquet/file/file_reader_test.go Outdated
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 5, 2023
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 5, 2023
@agchang agchang requested a review from zeroshade April 5, 2023 22:16
zeroshade
zeroshade reviewed Apr 6, 2023
View reviewed changes
Comment thread go/parquet/file/file_reader_test.go Outdated
Comment on lines +22 to +24
"github.com/apache/arrow/go/v12/parquet"
"github.com/apache/arrow/go/v12/parquet/schema"
"github.com/stretchr/testify/require"

@zeroshade zeroshade Apr 6, 2023

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.

can you run go fmt on this / goimports so it reorders the imports sorting them appropriately? that's likely what's causing the failing dev script CI tests, these are getting ordered differently resulting in the diffs not being what is expected.

@zeroshade zeroshade Apr 6, 2023

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.

I should really add a workflow for the bot to auto-run the formatting like we have for C++ and clang-format...

@agchang agchang Apr 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks for the pointer.

I should really add a workflow for the bot to auto-run the formatting like we have for C++ and clang-format...

I can take a look if you guys need help on the go front :-)

@agchang agchang Apr 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, still have failing checks, I ran:
go fmt -d file_reader.go file_reader_test.go and see no diffs 🤔

@zeroshade zeroshade Apr 10, 2023

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.

you needed to remove the empty line and then use goimports to sort the imports. I've done so and pushed the fix.

@agchang agchang Apr 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks for fixing.

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 6, 2023
@zeroshade

zeroshade commented Apr 6, 2023

Copy link
Copy Markdown
Member

@agchang the import order / go fmt issue is the only thing left on here, I'm happy to merge once that's addressed.

Thanks again for this!

@zeroshade zeroshade added this to the 12.0.0 milestone Apr 6, 2023
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 9, 2023
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 10, 2023
zeroshade
zeroshade approved these changes Apr 10, 2023
View reviewed changes

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

LGTM

@zeroshade zeroshade merged commit e50729a into apache:main Apr 10, 2023
@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Apr 10, 2023
@ursabot

ursabot commented Apr 10, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 2e6c13d and contender = e50729a. e50729a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.42% ⬆️0.06%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.22% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] e50729ac ec2-t3-xlarge-us-east-2
[Finished] e50729ac test-mac-arm
[Finished] e50729ac ursa-i9-9960x
[Finished] e50729ac ursa-thinkcentre-m75q
[Finished] 2e6c13d4 ec2-t3-xlarge-us-east-2
[Finished] 2e6c13d4 test-mac-arm
[Finished] 2e6c13d4 ursa-i9-9960x
[Finished] 2e6c13d4 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this pull request May 11, 2023
@agchang @zeroshade @liujiacheng777
apacheGH-34807: [Go] Handle io.EOF when reading parquet footer size…
17b34cf 
… and magic bytes (apache#34808)

* Closes: apache#34807

Lead-authored-by: Andy Chang <achang@arize.com>
Co-authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
@agchang @zeroshade @ArgusLi
apacheGH-34807: [Go] Handle io.EOF when reading parquet footer size…
44b4113 
… and magic bytes (apache#34808)

* Closes: apache#34807

Lead-authored-by: Andy Chang <achang@arize.com>
Co-authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zeroshade zeroshade zeroshade approved these changes

Assignees

No one assigned

Labels

awaiting merge Awaiting merge Component: Go

Projects

None yet

Milestone

12.0.0

Development

Successfully merging this pull request may close these issues.

[Go] file.NewParquetReader doesn't properly handle all io.ReaderAt implementations

3 participants

@agchang @zeroshade @ursabot