Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR: [Go] check nil buffer in SizeInBytes #42227

Merged
merged 4 commits into from
Jun 21, 2024
Merged

Conversation

Yifeng-Sigma
Copy link
Contributor

@Yifeng-Sigma Yifeng-Sigma commented Jun 20, 2024

Rationale for this change

image

Saw a nil error. Unclear to me how this nil buffer got constructed, because when building the arrow data we seem to already check it. But we should do a nil check here anyway

What changes are included in this PR?

Added a nil check.

Are these changes tested?

Are there any user-facing changes?

No.

@Yifeng-Sigma Yifeng-Sigma requested a review from zeroshade as a code owner June 20, 2024 19:16
Copy link

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:

@zeroshade zeroshade changed the title check nil buffer in SizeInBytes MINOR: [Go] check nil buffer in SizeInBytes Jun 20, 2024
@zeroshade
Copy link
Member

Looks good to me, can you add a unit test for this?

@Yifeng-Sigma
Copy link
Contributor Author

Looks good to me, can you add a unit test for this?

I tried but didn't figure out a way to build with nil buffer.
in NewData() we checked nil buffers.
image

@zeroshade
Copy link
Member

Creating an Array with no null values should produce something with a nil validity bitmap. Alternately you should be able to use NewData to create a Data object passing []*memory.Buffer{nil, buffer} since it's valid to have a nil validity bitmap if there are no nulls. I'll see if i can put together a unit test that reproduces the original crash that I can add for you

@zeroshade
Copy link
Member

@Yifeng-Sigma Adding the following to data_test.go will provide a test that should fail without this change and should succeed with this change:

image

@Yifeng-Sigma
Copy link
Contributor Author

@Yifeng-Sigma Adding the following to data_test.go will provide a test that should fail without this change and should succeed with this change:

image

Thanks! added to the PR.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Jun 21, 2024
@zeroshade zeroshade merged commit 77e572b into apache:main Jun 21, 2024
24 of 25 checks passed
Copy link

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 49 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants