wenhuitang commented on issue #800: [CALCITE-2460][CALCITE-2459] Add implementation of To_Base64 and From_Base64 to SqlFunctions URL: https://github.com/apache/calcite/pull/800#issuecomment-504404279 > Rather then `(temp.length() / 76) > 0` why not write `temp.length() > 76`? > > Punctuation consistent with surrounding code. Rather than `/** SQL TO_BASE64(string) function*/` write `/** SQL TO_BASE64(string) function. */` > > Rather than `assertEquals(expected, actual)` use `assertThat(actual, is(expected))`. Easier to read. > > Your algorithm creates a temp string and chews it down shorter and shorter. That is an O n^2 algorithm. Instead create one string, an offset into it (`for (int i = 0; i + 76 < s.length(); i += 76)`) and use `StringBuilder.append(s, i, i + 76)` or similar. Hello, @julianhyde Thanks for your time on this PR, I have addressed the comments, do you have time to have a look at the PR again?
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
