You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "don4get (via GitHub)" <gi...@apache.org> on 2024/08/07 14:23:39 UTC
[PR] [Go][Parquet] Recover from panic in file reader [arrow]
don4get opened a new pull request, #43607:
URL: https://github.com/apache/arrow/pull/43607
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) 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}
-->
### Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
The Parquet file reader should handle panics within its internal goroutines to ensure graceful failure and prevent client-side crashes.
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
`ReadRowGroups` now defers a panic recover from its goroutines and return an error to gracefully fail file reading.
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
No more tests were added because I cannot reproduce such a corrupted file with dummy data.
### Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
**This PR contains a "Critical Fix".**
* Github issue: https://github.com/apache/arrow/issues/43605
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2307457857
Sigh, I would check ARROW-GH-43605.parquet for now...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2273735308
Do we know why it was causing the panic in the first place? Can you add the corrupted file to https://github.com/apache/arrow-testing and then use that for a test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #43607:
URL: https://github.com/apache/arrow/pull/43607#discussion_r1734846916
##########
go/parquet/pqarrow/file_reader_test.go:
##########
@@ -373,3 +374,43 @@ func TestFileReaderColumnChunkBoundsErrors(t *testing.T) {
assert.ErrorContains(t, tooHighErr, fmt.Sprintf("there are only %d columns", schema.NumFields()))
}
}
+
+func TestReadParquetFile(t *testing.T) {
+ dir := os.Getenv("PARQUET_TEST_BAD_DATA")
+ if dir == "" {
+ t.Skip("no path supplied with PARQUET_TEST_BAD_DATA")
+ }
+ assert.DirExists(t, dir)
+ filename := path.Join(dir, "ARROW-GH-43605.parquet")
+ ctx := context.TODO()
+
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+
+ rdr, err := file.OpenParquetFile(
+ filename,
+ false,
+ file.WithReadProps(parquet.NewReaderProperties(mem)),
+ )
+ if err != nil {
+ t.Errorf("unexpected error: %v", err)
+ }
+ defer func() {
+ if err2 := rdr.Close(); err2 != nil {
+ t.Errorf("unexpected error: %v", err2)
+ }
+ }()
+
+ arrowRdr, err := pqarrow.NewFileReader(rdr, pqarrow.ArrowReadProperties{
+ Parallel: false,
+ BatchSize: 0,
+ }, mem)
+ if err != nil {
+ t.Errorf("unexpected error: %v", err)
+ }
+
+ _, err = arrowRdr.ReadTable(ctx)
+
+ if err == nil {
+ t.Errorf("expected error: %v", err)
+ }
Review Comment:
`assert.Error(t, err)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2346773088
As the Go implementation has shifted to the github.com/apache/arrow-go repository, once all the CI here passes, I'm going to shift this to the new repo and merge it there instead as we are no longer merging Go PRs on this repo. (We've only held off on shifting ALL the Go PRs while we get the CI fully complete over there).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2273604011
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) 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:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2295827744
I'll try to figure out it later, you can try to read another "bad_data" file firstly
I believe it's easy to re-producing with other bad data file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #43607:
URL: https://github.com/apache/arrow/pull/43607#discussion_r1734845531
##########
go/parquet/pqarrow/file_reader_test.go:
##########
@@ -373,3 +374,43 @@ func TestFileReaderColumnChunkBoundsErrors(t *testing.T) {
assert.ErrorContains(t, tooHighErr, fmt.Sprintf("there are only %d columns", schema.NumFields()))
}
}
+
+func TestReadParquetFile(t *testing.T) {
+ dir := os.Getenv("PARQUET_TEST_BAD_DATA")
+ if dir == "" {
+ t.Skip("no path supplied with PARQUET_TEST_BAD_DATA")
+ }
+ assert.DirExists(t, dir)
+ filename := path.Join(dir, "ARROW-GH-43605.parquet")
+ ctx := context.TODO()
+
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+
+ rdr, err := file.OpenParquetFile(
+ filename,
+ false,
+ file.WithReadProps(parquet.NewReaderProperties(mem)),
+ )
+ if err != nil {
+ t.Errorf("unexpected error: %v", err)
+ }
Review Comment:
`require.NoError(t, err)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2293078804
Does this is real bad-data? I found my parquet-c++ can read this out ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2346770750
@don4get I figured out the issue and uploaded a fix so that the tests should all work properly now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2346954057
> @don4get I figured out the issue and uploaded a fix so that the tests should all work properly now.
Thanks @zeroshade :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2316924076
The test fails when this flag is set: `-tags noasm`. I don't know what it changes to the code base.
I can suggest to modify the test and remove the condition on the last error.
The main point of this PR is to prevent a panic coming from another goroutine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2307520865
testing file is merged, sorrying for delaying to find the reason. just go-through for testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2273784101
> Do we know why it was causing the panic in the first place? Can you add the corrupted file to https://github.com/apache/arrow-testing and then use that for a test?
Sorry but I cannot share this corrupted file. I tried to reproduce a dummy file reproducing the bug but I did not find a way to corrupt the dummy file the same way as the original one because I do not know the corruption cause.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2275859989
New PR dealing with the cause of the panic is opened:
https://github.com/apache/arrow/pull/43616
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2273789279
https://github.com/apache/parquet-testing/pull/48 have some corrput parquet file which is able to use, but it might need a few days to check and merge that. Maybe it's more easier to makeup a test with go writer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2295824752
> Does this is real bad-data? I found my parquet-c++ can read this out ?
Maybe it should not be considered as bad data but arrow-go did not manage to open it before my PR.
Should I move it to "DATA" folder then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2307473894
> Sigh, I would check ARROW-[GH-43605](https://github.com/apache/arrow/issues/43605).parquet for now...
Concerning the panics caused by #41317 and #41321, these are not urgent matter because they are in the same thread than the `ReadTable` call, which means users can simply handle the panic on their side. I would say it is cleaner to simply return an error instead of panicking. Yet since they can handle it on their side, it is not urgent.
For the issue I am pointing in #43605, the problem is more severe because the panic happens in a goroutine, it is not handled by arrow and the user simply cannot handle it. User program will crash, no workaround possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2307454363
> > I believe it's easy to re-producing with other bad data file
>
> Would you mind read other bad-data?
`ARROW-GH-41317` causes another panic which should be handled differently than it is in this PR:
More info in [panic_while_reading_ARROW-GH-41317.txt](https://github.com/user-attachments/files/16731544/panic_while_reading_ARROW-GH-41317.txt)
`panic_while_reading_ARROW-GH-41321` causes yet another panic which should be handled like for #41317, in [ReadTable](https://github.com/don4get/arrow/blob/8e034c7d1051b12d24f9e26ec9ef2201af27c526/go/parquet/pqarrow/file_reader.go#L264).
Other files have been added 6 years ago, I doubt they are relevant for the issue I am targeting in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #43607:
URL: https://github.com/apache/arrow/pull/43607#discussion_r1734846159
##########
go/parquet/pqarrow/file_reader_test.go:
##########
@@ -373,3 +374,43 @@ func TestFileReaderColumnChunkBoundsErrors(t *testing.T) {
assert.ErrorContains(t, tooHighErr, fmt.Sprintf("there are only %d columns", schema.NumFields()))
}
}
+
+func TestReadParquetFile(t *testing.T) {
+ dir := os.Getenv("PARQUET_TEST_BAD_DATA")
+ if dir == "" {
+ t.Skip("no path supplied with PARQUET_TEST_BAD_DATA")
+ }
+ assert.DirExists(t, dir)
+ filename := path.Join(dir, "ARROW-GH-43605.parquet")
+ ctx := context.TODO()
+
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+
+ rdr, err := file.OpenParquetFile(
+ filename,
+ false,
+ file.WithReadProps(parquet.NewReaderProperties(mem)),
+ )
+ if err != nil {
+ t.Errorf("unexpected error: %v", err)
+ }
+ defer func() {
+ if err2 := rdr.Close(); err2 != nil {
+ t.Errorf("unexpected error: %v", err2)
+ }
+ }()
+
+ arrowRdr, err := pqarrow.NewFileReader(rdr, pqarrow.ArrowReadProperties{
+ Parallel: false,
+ BatchSize: 0,
+ }, mem)
+ if err != nil {
+ t.Errorf("unexpected error: %v", err)
+ }
Review Comment:
`require.NoError(t, err)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2316863946
> Looks like the last call in the unit test is getting `nil` when it expected an error, is that a problem with the test or the fix?
I manage to reproduce this locally with `./ci/scripts/go_test.sh`. However, when I call the test directly, either from IDE or command line, the test is OK:
```
PARQUET_TEST_BAD_DATA=/home/xxx/work/arrow-1/cpp/submodules/parquet-testing/bad_data go test -asan -timeout 30s -run ^TestReadParquetFile$ github.com/apache/arrow/go/v18/parquet/pqarrow
```
I initially thought it was linked to `-asan` flag, but it's not like this. Do you have an idea what is the difference between the
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2302418818
@don4get the parquet-testing PR was merged, can you update the submodule here and push it so it can run the CI with the new file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2307497885
I got this! Also, can you provide this link in polaris about how this issue is fixed in polaris? I cannot find that now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2273608723
:warning: GitHub issue #43605 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2307184768
> I believe it's easy to re-producing with other bad data file
Would you mind read other bad-data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2307479248
> Sigh, I would check `ARROW-GH-43605.parquet` for now...
>
> I still don't know, since this pr is just recover from panic, #43616 is related to the avx2 decoding in `ARROW-GH-43605.parquet`. Can we using other file in this patch? Or this is strongly related to `ARROW-GH-43605.parquet` and cannot be reproduce with other file?
#43605 highlights a panic in a goroutine, which in the end is a crash in avx2 decoding.
#41317 leads to a panic in [validate](https://github.com/don4get/arrow/blob/8e034c7d1051b12d24f9e26ec9ef2201af27c526/go/arrow/array/table.go#L226), far from avx decoding.
It is not the same issue, I cannot reuse these files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2330272997
What is the error that you get when running it locally without the `noasm` flag? Did you expect there to be an error?
That particular flag controls whether or not optimized assembly code is utilized for decoding values, when that flag is set the library will fall back to pure go implementations of several decoding aspects such as unpacking booleans or packed integers. Without that flag, SIMD (vectorized) implementations of those functions are used instead on a per-architecture basis (falling back to pure go if we don't have optimized assembly for a given platform).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2275701780
I managed to reproduce the dummy failing data.
I is produced with [polars](https://github.com/pola-rs/polars) with this version:
```
[[package]]
name = "polars"
version = "0.20.13"
description = "Blazingly fast DataFrame library"
optional = false
python-versions = ">=3.8"
files = [
{file = "polars-0.20.13-cp38-abi3-macosx_10_12_x86_64.whl", hash = "sha256:63bb00eb32b151a949666f8ae050bd09159bca572c0eab17a17abe6ac50fc8e0"},
{file = "polars-0.20.13-cp38-abi3-macosx_11_0_arm64.whl", hash = "sha256:4ed694808252968dcda486dcd6009b8230bada83924d4f5d3359dbe3db6ab8e5"},
{file = "polars-0.20.13-cp38-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:1557e8947a263cefc1937cd047800678fbae8c9a475e6dada5b7dc6557180a4f"},
{file = "polars-0.20.13-cp38-abi3-manylinux_2_24_aarch64.whl", hash = "sha256:8694d6fc307256e9e36b03975ccc89ce89290d6d661f75eb60e14e304d1e0968"},
{file = "polars-0.20.13-cp38-abi3-win_amd64.whl", hash = "sha256:3917868d0a0331436a426f7acda24b2806e7f2458ee91f581d44765c9e87abe8"},
{file = "polars-0.20.13.tar.gz", hash = "sha256:b3115c7499705d8f1a790add5806747a2eb3f19660d277e8e823199dcb66aeaf"},
]
```
It's a single column uint16 dataset, filled with 0 values.
The same dataset using polars with the following version does not reproduce the bug:
```
[[package]]
name = "polars"
version = "1.4.1"
description = "Blazingly fast DataFrame library"
optional = false
python-versions = ">=3.8"
files = [
{file = "polars-1.4.1-cp38-abi3-macosx_10_12_x86_64.whl", hash = "sha256:f02fc6a5c63dd86cfeb159caa66112e477c69fc7800a28e64609ac2780554865"},
{file = "polars-1.4.1-cp38-abi3-macosx_11_0_arm64.whl", hash = "sha256:bd2acd8b1977f61b9587c8d47d16f101e7e73edd8cdeb3a8a725f15f181cd120"},
{file = "polars-1.4.1-cp38-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:7cf834a328e292c31c06eb606496becb6d8a795e927c826e26e2af27087950f1"},
{file = "polars-1.4.1-cp38-abi3-manylinux_2_24_aarch64.whl", hash = "sha256:64eabf0ef7ac0d17fe15361e7daaeb4425a875d2d760c17d96803e9ac8bee244"},
{file = "polars-1.4.1-cp38-abi3-win_amd64.whl", hash = "sha256:2313d63ecfa1d9f1e740b9fcabb8ae45d9d0b5acf1ddb401951daba4c0f3f74f"},
{file = "polars-1.4.1.tar.gz", hash = "sha256:ed8009aff8cf91f94db5a38d947185603ad5bee48a28b764cf5a52048c7c4756"},
]
```
To be able to execute this test, we now need to wait for this other PR to be merged:
https://github.com/apache/parquet-testing/pull/57
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2315600177
Looks like the last call in the unit test is getting `nil` when it expected an error, is that a problem with the test or the fix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2307100951
> @don4get the parquet-testing PR was merged, can you update the submodule here and push it so it can run the CI with the new file?
Hello, the PR needed for testing has been approved but not merged:
https://github.com/apache/parquet-testing/pull/57/
Can you please merge it? I will update this PR once it is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2309655336
> I got this! Also, can you provide this link in polaris about how this issue is fixed in polaris? I cannot find that now
I don't know yet what fixed the issue in `polars`, I can only say for now that `v1.4.1` is fixed, where as `v0.20.13` caused the issue. I eventually investigate :)
> testing file is merged, sorrying for delaying to find the reason. just go-through for testing
Thank you so much. I just updated the submodule both in the branch and the [other one](https://github.com/apache/arrow/pull/43616).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "don4get (via GitHub)" <gi...@apache.org>.
don4get commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2345983053
> What is the error that you get when running it locally without the `noasm` flag? Did you expect there to be an error?
>
> That particular flag controls whether or not optimized assembly code is utilized for decoding values, when that flag is set the library will fall back to pure go implementations of several decoding aspects such as unpacking booleans or packed integers. Without that flag, SIMD (vectorized) implementations of those functions are used instead on a per-architecture basis (falling back to pure go if we don't have optimized assembly for a given platform).
```
file_reader_test.go:409:
Error Trace: /arrow/go/parquet/pqarrow/file_reader_test.go:409
Error: An error is expected but got nil.
```
```
_, err = arrowRdr.ReadTable(ctx)
assert.Error(t, err)
```
https://github.com/don4get/arrow/blob/589c3e4df00ca906c65830fa4e90ca182f424276/go/parquet/pqarrow/file_reader_test.go#L409
While the default implementation fails to read the table (it returns an error), the `noasm` version does not fail (does not return an error).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade closed pull request #43607: GH-43605: [Go][Parquet] Recover from panic in file reader
URL: https://github.com/apache/arrow/pull/43607
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43605: [Go][Parquet] Recover from panic in file reader [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #43607:
URL: https://github.com/apache/arrow/pull/43607#issuecomment-2349654115
Closing this to move it to https://github.com/apache/arrow-go/pull/124 for the new Arrow Go repo.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org