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

SQLLine uses default encoding which is hard to predict when the library is embedded #155

Closed
vlsi opened this issue Sep 18, 2018 · 6 comments

Comments

@vlsi
Copy link

vlsi commented Sep 18, 2018

private PrintStream outputStream = new PrintStream(System.out, true);
private PrintStream errorStream = new PrintStream(System.err, true);

this.outputStream = new PrintStream(outputStream, true);

this.errorStream = new PrintStream(errorStream, true);

@julianhyde
Copy link
Owner

fobiddenapis could probably catch this. (We use it in Calcite to remind us to call String.toUpper(Locale.ROOT) rather than String.toUpper().)

@snuyanzin
Copy link
Collaborator

snuyanzin commented Oct 1, 2018

There is an option to use something like java.nio.charset.StandardCharsets for instance

private PrintStream outputStream = new PrintStream(System.out, true, StandardCharsets.UTF_8);

I think it may also make sense to add variable default-character-set like the existing in MySQL [1] or PostgreSQL[2].
The issue is that java.nio.charset.StandardCharsets is accessible since jdk 1.7 so what about moving to 1.8 with upgrading to jline3 first?
[1] https://dev.mysql.com/doc/refman/5.7/en/mysql-command-options.html#option_mysql_default-character-set
[2] https://www.postgresql.org/docs/9.3/static/runtime-config-client.html#GUC-CLIENT-ENCODING

@julianhyde
Copy link
Owner

StandardCharsets makes sense. We did the same in https://issues.apache.org/jira/browse/CALCITE-1667.

Yes, let's do JDK 8 and jline3 first. As soon as I get a moment I will review/merge that change, then we can revisit this issue.

@snuyanzin
Copy link
Collaborator

As I understand committed at f4bfb5c with adding forbiddenapis and fixing all the errors based on its report

@snuyanzin
Copy link
Collaborator

cc @julianhyde
If I am not mistaken it could closed as fixed

@julianhyde
Copy link
Owner

Fixed in f4bfb5c. Thanks for reminding me, @snuyanzin!

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

No branches or pull requests

3 participants