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

Remove heading null chars to prevent load errors #17

Merged
merged 5 commits into from Dec 21, 2016

Conversation

shimpeko
Copy link
Collaborator

Change text operator to remove heading \0 (null char) which cause load error with Redshift.

@aamine please review

@@ -65,4 +65,15 @@ String castStringForce(Object value) {
if (!(value instanceof String)) return null;
return (String)value;
}

final Pattern nullCharsPattern = Pattern.compile("^\\x00+$");
String removeHeadNullChars(String str) {
Copy link
Member

Choose a reason for hiding this comment

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

Why removing only heading characters instead of whole text? It does not make sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought \0hoge should be hoge. Because I thought it's better to preserve as much data as possible in this phase and allow users to choose whether ignore this data or not.

Why whole text should be dropped? Is that because \0hoge and hoge are not same? How about change to \\0hoge?

final Pattern nullCharsPattern = Pattern.compile("^\\x00+$");
String removeHeadNullChars(String str) {
if (str == null) return null;
// return if first char is not \0
Copy link
Member

Choose a reason for hiding this comment

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

redundant comment

if (str == null) return null;
// return if first char is not \0
if (0 != str.indexOf("\0")) return str;
// return null if all chars are \0
Copy link
Member

Choose a reason for hiding this comment

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

redundant comment

if (0 != str.indexOf("\0")) return str;
// return null if all chars are \0
if (nullCharsPattern.matcher(str).matches()) return null;
// remove heading \0(s) if some chars follows
Copy link
Member

Choose a reason for hiding this comment

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

redundant comment

@@ -65,4 +65,15 @@ String castStringForce(Object value) {
if (!(value instanceof String)) return null;
return (String)value;
}

final Pattern nullCharsPattern = Pattern.compile("^\\x00+$");
Copy link
Member

Choose a reason for hiding this comment

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

insert blank line after this

// return null if all chars are \0
if (nullCharsPattern.matcher(str).matches()) return null;
// remove heading \0(s) if some chars follows
return str.replaceFirst("^\\x00+", "");
Copy link
Member

Choose a reason for hiding this comment

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

  1. Why not compile this pattern (while above pattern is compiled)?
  2. Replace first, then check the string is empty (and return null if empty).

String removeHeadNullChars(String str) {
if (str == null) return null;
// return if first char is not \0
if (0 != str.indexOf("\0")) return str;
Copy link
Member

Choose a reason for hiding this comment

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

Flipped condition expression (invert of str.indexOf("\0") != 0) is a kind of superstition. Never use that.

@shimpeko
Copy link
Collaborator Author

@aamine please

Copy link
Member

@aamine aamine left a comment

Choose a reason for hiding this comment

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

I think deleting NUL and all following characters (without condition) is clean and consistent way. Special handling for heading NUL seems cludge.

@shimpeko
Copy link
Collaborator Author

Fixed.

Copy link
Member

@aamine aamine left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -65,4 +65,12 @@ String castStringForce(Object value) {
if (!(value instanceof String)) return null;
return (String)value;
}

Pattern afterNullCharPattern = Pattern.compile("\\x00.*");
Copy link
Member

Choose a reason for hiding this comment

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

[nits] Constants should be upper chars

@shimpeko
Copy link
Collaborator Author

plz merge

@@ -65,4 +65,12 @@ String castStringForce(Object value) {
if (!(value instanceof String)) return null;
return (String)value;
}

final private Pattern AFTER_NULL_CHAR_PATTERN = Pattern.compile("\\x00.*");
Copy link
Member

Choose a reason for hiding this comment

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

はっ…… static
あとprivateは滅多につけなくてよいです、むしろ何も付けずにpackage privateにするほうがいい(何も付けないとデフォルトでpackage privateになるということは、利点が大きいからデフォルトにしてあるのです一般的に言って)

@shimpeko
Copy link
Collaborator Author

fixed

@aamine aamine merged commit b6c44ba into bricolages:master Dec 21, 2016
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.

None yet

2 participants