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

Proposal: remove all try-catch style codes #457

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hangxie
Copy link
Contributor

@hangxie hangxie commented Apr 17, 2022

This is to follow up #395, all existing recover calls are doing Java/C++ try-catch style error handling, which makes things hard to debug due to lack of call stack.

For example, #452 reports problem without any source file and line numbers, after removing all recover calls, I got:

$ go run type_alias.go
2022/04/17 08:17:13 Write Finished
panic: reflect.Set: value of type int32 is not assignable to type main.AgeInt

goroutine 33 [running]:
reflect.Value.assignTo({0x104d25680?, 0x104eddc48?, 0x104d480c0?}, {0x104ca6af7, 0xb}, 0x104d25800, 0x0)
	/opt/homebrew/Cellar/go/1.18.1/libexec/src/reflect/value.go:3062 +0x210
reflect.Value.Set({0x104d25800?, 0x14000283258?, 0x0?}, {0x104d25680?, 0x104eddc48?, 0x0?})
	/opt/homebrew/Cellar/go/1.18.1/libexec/src/reflect/value.go:2088 +0xcc
github.com/xitongsys/parquet-go/marshal.Unmarshal(0x140002846c8, 0x6, 0x9, {0x104d1ae60?, 0x14000283200}, 0x140002a5c70, {0x0, 0x0})
	/Users/xiehang/dev/parquet-go/marshal/unmarshal.go:228 +0xc84
github.com/xitongsys/parquet-go/reader.(*ParquetReader).read.func2(0x0?, 0x0?, 0x2)
	/Users/xiehang/dev/parquet-go/reader/reader.go:336 +0x11c
created by github.com/xitongsys/parquet-go/reader.(*ParquetReader).read
	/Users/xiehang/dev/parquet-go/reader/reader.go:330 +0x4c8
exit status 2

which makes it super easy to fix.

This PR may cause panic for those who depend on parquet-go in near future, but we can collect all those panic cases and fix them by adding extra check on incoming data/parameters.

I used patched version with my personal project, it just works without any problems.

@hangxie hangxie force-pushed the avoid-recover-from-panic branch 2 times, most recently from 6eef0c4 to 3c0bfc2 Compare April 23, 2022 14:15
@hangxie hangxie force-pushed the avoid-recover-from-panic branch 2 times, most recently from 1839277 to 2e8e602 Compare May 28, 2022 01:43
zolstein pushed a commit to zolstein/parquet-go that referenced this pull request Jun 23, 2023
* add parquet.RowBuilder

* fixes for parquet row generation

* add parquet.(*RowBuilder).Next

* move all merge code to merge.go (xitongsys#458)

* move all merge code to merge.go

* add example

* add benchmark for parquet.(*RowBuilder).Add

* initialize FIXED_LEN_BYTE_ARRAY columns to non-null zero-value

* make it OK to call Next if the repetition level is zero
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants