spark icon indicating copy to clipboard operation
spark copied to clipboard

[SPARK-48219][CORE] StreamReader Charset fix with UTF8

Open xuzifu666 opened this issue 1 year ago • 5 comments

What changes were proposed in this pull request?

Fix some StreamReader not set with UTF8,if we actually default charset not support Chinese chars such as latin and conf contain Chinese chars,it would not resolve success,so we need set it as utf8 in StreamReader,we can find all StreamReader with utf8 charset in other compute framework,such as Calcite、Hive、Hudi and so on.

Why are the changes needed?

May cause string decode not as expected

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

Not need

Was this patch authored or co-authored using generative AI tooling?

No

xuzifu666 avatar May 09 '24 13:05 xuzifu666

Do you think you can provide a test coverage to protect your contribution from potential future regression, @xuzifu666 ?

Not need

@dongjoon-hyun Thanks for your attentions,In my option this code change not need to provide tests for it's a specification for ReadStream usage,if not set utf8 charset may occur error when system default charset not contains Chinese Chars. You can refer it in other framework such as Calcite,Hive,all set utf8 when InputStreamReader constructor method be called.

xuzifu666 avatar May 10 '24 01:05 xuzifu666

@dongjoon-hyun Could you give a final review? Thanks

xuzifu666 avatar May 10 '24 05:05 xuzifu666

Sorry but I'll leave this to the other reviewers, @xuzifu666 .

dongjoon-hyun avatar May 10 '24 05:05 dongjoon-hyun

@HyukjinKwon could you help to give a review? Thanks

xuzifu666 avatar May 10 '24 06:05 xuzifu666

The change itself looks reasonable to me. I also agree with @dongjoon-hyun that we shall add a simple test, maybe in XSDToSchemaSuite.

BTW, the PR is tagged as CORE but the changes belong to XML of sql datasource and hive thriftserver

yaooqinn avatar May 10 '24 08:05 yaooqinn

XSDtoSchema would not modify it, than HiveImpl had also changed can refer recent pr: https://github.com/apache/hive/pull/5243 so I Think it is nesscery to change it? @yaooqinn @HyukjinKwon

xuzifu666 avatar May 16 '24 04:05 xuzifu666

Thank you @xuzifu666.

Merged to master

yaooqinn avatar May 16 '24 04:05 yaooqinn