Skip to content

Core: Fix unicode handling in HTTPClient#8046

Merged
danielcweeks merged 1 commit into
apache:masterfrom
nastra:string-entity-encoding
Jul 13, 2023
Merged

Core: Fix unicode handling in HTTPClient#8046
danielcweeks merged 1 commit into
apache:masterfrom
nastra:string-entity-encoding

Conversation

@nastra

@nastra nastra commented Jul 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

fixes #7821

Without the fix, tests would fail with

expected: struct<1: id: required int (unique ID 🤪), 2: data: required string>
 but was: struct<1: id: required int (unique ID ?), 2: data: required string>

@github-actions github-actions Bot added the core label Jul 12, 2023
@nastra nastra mentioned this pull request Jul 12, 2023
Closed
@nastra nastra force-pushed the string-entity-encoding branch from 1c3126d to 34d262f Compare July 12, 2023 08:54
singhpk234
singhpk234 approved these changes Jul 12, 2023
View reviewed changes

@singhpk234 singhpk234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, Thanks @nastra !

dramaticlly
dramaticlly approved these changes Jul 12, 2023
View reviewed changes

@dramaticlly dramaticlly left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, maybe also worth updating this hardcoded UTF-8 in PartitionSpec to using StandardCharsets as well? https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L174

@nastra
Core: Fix unicode handling in HTTPClient
2c6e9ab 
fixes apache#7821

Without the fix, tests would fail with
```
expected: struct<1: id: required int (unique ID 🤪), 2: data: required string>
 but was: struct<1: id: required int (unique ID ?), 2: data: required string>
```
@nastra nastra force-pushed the string-entity-encoding branch from 34d262f to 2c6e9ab Compare July 13, 2023 06:23
@nastra

nastra commented Jul 13, 2023

Copy link
Copy Markdown
Contributor Author

LGTM, maybe also worth updating this hardcoded UTF-8 in PartitionSpec to using StandardCharsets as well? https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L174

Seems like there are a few more places that use the hardcoded string and it's not directly related to this PR. @dramaticlly would you be interested in updating those?

@dramaticlly

dramaticlly commented Jul 13, 2023

Copy link
Copy Markdown
Contributor

LGTM, maybe also worth updating this hardcoded UTF-8 in PartitionSpec to using StandardCharsets as well? https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L174

Seems like there are a few more places that use the hardcoded string and it's not directly related to this PR. @dramaticlly would you be interested in updating those?

Sure I can cut a separate PR to update those.

@danielcweeks danielcweeks merged commit 9572585 into apache:master Jul 13, 2023
@nastra nastra deleted the string-entity-encoding branch July 14, 2023 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danielcweeks danielcweeks danielcweeks approved these changes
@singhpk234 singhpk234 singhpk234 approved these changes
+1 more reviewer
@dramaticlly dramaticlly dramaticlly approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[bug] Encountered Chinese garbled code issue during Iceberg rest HTTP transmission

4 participants

@nastra @dramaticlly @danielcweeks @singhpk234