[GitHub] phoenix issue #275: PHOENIX-4237: Add function to calculate Java collation k...
Github user snakhoda-sfdc commented on the issue: https://github.com/apache/phoenix/pull/275 @JamesRTaylor I'm not sure how to do that within this PR. Looking at https://github.com/blog/2141-squash-your-commits, I believe at the time you merge the PR, github should give you the option to squash all commits into one. Will that suffice? ---
[GitHub] phoenix issue #275: PHOENIX-4237: Add function to calculate Java collation k...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/275 Would you mind squashing all the commits into a single commit, @shehzaadn and I'll get this committed? ---
[GitHub] phoenix issue #275: PHOENIX-4237: Add function to calculate Java collation k...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/275 +1. Nice work, @shehzaadn! ---
[GitHub] phoenix issue #275: PHOENIX-4237: Add function to calculate Java collation k...
Github user snakhoda-sfdc commented on the issue: https://github.com/apache/phoenix/pull/275 @JamesRTaylor i've addressed the last round of comments in this commit (9d6d4f7). Thanks. ---
[GitHub] phoenix issue #275: PHOENIX-4237: Add function to calculate Java collation k...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/275 Looking very good. Couple minor nits and the testing needs to be rounded out just a bit. ---
[GitHub] phoenix issue #275: PHOENIX-4237: Add function to calculate Java collation k...
Github user snakhoda-sfdc commented on the issue: https://github.com/apache/phoenix/pull/275 @JamesRTaylor thanks for the feedback and support! So we have the i18n-util jar on maven now, but not the icu4j jars. Once the icu4j jars are published to maven, i18n-util will have to change to upgrade its dependency to the new version. I'm hoping that change will be in next week. Once that happens, I was thinking of creating a new PR that removes the outside code and introduces the external dependency. ---
[GitHub] phoenix issue #275: PHOENIX-4237: Add function to calculate Java collation k...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/275 This is looking very good, @shehzaadn - thanks for the revisions. Couple more comments, but it's getting pretty close IMHO. How is the publishing to maven of the dependent jars looking? ---
[GitHub] phoenix issue #275: PHOENIX-4237: Add function to calculate Java collation k...
Github user snakhoda-sfdc commented on the issue: https://github.com/apache/phoenix/pull/275 @JamesRTaylor Thanks for your comments. I added two further commits: 199c389: This addresses your comment about the byte array comparison. You were right! I must have got confused earlier with what was being displayed on sqlline.py not matching the sort order. I also added a formatter for PVarBinary because without it you simply get a Java hash code in sqlline.py which is hard to do anything with. 8cc2b5c: This adds the end-to-end tests you mentioned and also changes the unit test to use the hex representation of the byte array to make it easier to read. ---
[GitHub] phoenix issue #275: PHOENIX-4237: Add function to calculate Java collation k...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/275 Thanks for the patch, @shehzaadn. This looks like a general enough built-in function to include in Phoenix IMHO. See inline for more specific comments. It'd be much better to include the first two commits as external dependencies. If we don't do that, we'll need to quickly follow up with replacing them with external dependencies (and make sure we don't change those files at all). ---
