Skip to content

[BEAM-3348] Set utf-16 as SQL DSL default charset#4258

Merged
mingmxu merged 1 commit intoapache:masterfrom
Alienero:master
Dec 29, 2017
Merged

[BEAM-3348] Set utf-16 as SQL DSL default charset#4258
mingmxu merged 1 commit intoapache:masterfrom
Alienero:master

Conversation

@Alienero
Copy link
Contributor

@Alienero Alienero commented Dec 14, 2017

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@Alienero
Copy link
Contributor Author

Alienero commented Dec 20, 2017

R: @xumingmin

@mingmxu mingmxu requested a review from xumingming December 20, 2017 18:10
@mingmxu mingmxu self-assigned this Dec 20, 2017
@mingmxu
Copy link

mingmxu commented Dec 20, 2017

retest this please

@Alienero Alienero force-pushed the master branch 3 times, most recently from ccca777 to b0b8e4e Compare December 21, 2017 06:53
@Alienero
Copy link
Contributor Author

Alienero commented Dec 21, 2017

hi @xumingmin , I hava run mvn clean verify in beam/sdks/java/extensions/sql, all unit test passed. please take a look

@Alienero
Copy link
Contributor Author

retest this please.

@robertwb
Copy link
Contributor

What's the motivation for UTF-16? Everywhere else we seem to use UTF-8.

Copy link

@mingmxu mingmxu left a comment

Choose a reason for hiding this comment

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

Given that Calcite only support UTF16 and ISO-8859-1(LATIN1)[1], this sounds a good option in SQL component, and it's also the solution in other projects like Flink[2].

@Alienero there're another two properties in SaffronProperties.java, is it safe to ignore them?

[1]. http://mail-archives.apache.org/mod_mbox/calcite-dev/201710.mbox/%3CCAPSgeETv5aJ3GKAkoYhCN7oihu9cNLwwLFrs7AsTmC-9WTk2aw@mail.gmail.com%3E
[2]. https://issues.apache.org/jira/browse/FLINK-7451

feat: support national charset
@Alienero
Copy link
Contributor Author

Alienero commented Dec 23, 2017

hi @xumingmin,I made some additions: add default national charset and collation. uft-16 also as default charset for the N'string' construct

R: @xumingmin

@mingmxu
Copy link

mingmxu commented Dec 26, 2017

retest this please

Copy link

@mingmxu mingmxu left a comment

Choose a reason for hiding this comment

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

LGTM, merging

@mingmxu
Copy link

mingmxu commented Dec 29, 2017

retest this please

@mingmxu mingmxu merged commit 845e22f into apache:master Dec 29, 2017
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