-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Remove fastjson library #28007
Remove fastjson library #28007
Conversation
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
Assigning reviewers. If you would like to opt out of this review, comment R: @bvolpato for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Thanks for working on this! There are merge conflicts - could you please rebase onto the current master? Also, consider add this change to CHANGES.md. |
...ava/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BigQueryTableProviderTest.java
Outdated
Show resolved
Hide resolved
added |
Run SQL PostCommit |
There are test failures:
Previously a ill-formed json Similarly, it is nice to tolerant trailing comma |
I will add |
Thanks again for the fix. It appears fastjson parser in general is more relax than the default jackson mapper. Let me do some small experiments regarding the default behavior of two parsers, specifically, the list in https://fasterxml.github.io/jackson-core/javadoc/2.9/com/fasterxml/jackson/core/JsonParser.Feature.html |
Experiment result shows fastjson in general is much more relax in non-standard syntax. We may need to add other configs, see below.
|
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/TableUtils.java
Outdated
Show resolved
Hide resolved
…ensions/sql/TableUtils.java more configs to default behaviour Co-authored-by: Yi Hu <[email protected]>
Run SQL PostCommit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fix #24154,
hi @apilloud can you help to review this PR?