PHOENIX-6818 Remove dependency on the i18n-util library

[symat]

i18n-util is not maintained anymore, but uses icu4j dependencies having CVE issues. To avoid these problems, I copied the relevant code from i18n-util and used the latest icu4j version.

[gjacoby126]

Thanks for moving these into Phoenix. Just a question about whether we need all of these files.

[gjacoby126]

Are the OracleUpper, OracleUpperTable, and OracleUpperTableGenratorTest actually used within Phoenix? I just see references to LocaleUtils and LinguisticSort in Phoenix, and I don't see a reference to the three Oracle* classes from either of those. Did I miss a dependency?

[symat]

thanks for checking!

yes, all these OracleUpper related shenanigans are needed if in the CollatinKeyFunction we go with linguisticSort.getUpperCaseCollator:

phoenix/phoenix-core/src/main/java/org/apache/phoenix/expression/function/CollationKeyFunction.java

Lines 159 to 160 in 8820d69

	collator = BooleanUtils.isTrue(useSpecialUpperCaseCollator) ? linguisticSort.getUpperCaseCollator(false)
	: linguisticSort.getCollator();

In this case the LinguisticSort is using these OracleUpper classes.

I also missed this on the first look, and it made the patch a bit more complex than how I originally thought.

Actually the OracleUpperTableGenratorTest could be removed. That is not really a test, but a code generator utility in disguise. I just left it there because I thought if we need to fix anything in OracleUpperTable later, then maybe we can use that generator again. But it is rather unlikely that we will ever need this in Phoenix. What do you think?

[stoty]

I'd prefer the pulled in code to live its own package, i.e org.apache.phoenix.i18nutil.

Update: Now I see that most of it is already under org.apache.phoenix.i18. That's fine.

[symat]

yes, the only "copied" code which I put directly to org.phoenix.util was the DeferredStringBuilder which has nothing to do to internationalization. And also might be handy for other purposes. But we can move that to org.phoenix.util.i18n too, if you prefer that.

[stoty]

+1 LGTM but let's wait for Geoffrey's response before pushing.

[gjacoby126]

@symat - if we keep the code generator, let's change it to output the Apache license rather than the Salesforce one. Other than that, lgtm.

[symat]

@stoty, @gjacoby126, thank you both for the quick reviews!!

let's change it to output the Apache license rather than the Salesforce one

nice catch, I fixed this.

[stoty]

Yetus doesn't like the ASF license formatting in a few files.
It also complains about the author tags.

Please address the checkstyle and author checks above. (actually, not it's below)

[symat]

Thank you!

I went through the problems reported by yetus. Some of them were valid, but many of them were already fixed in the previous commits. Anyway, let's see if the checks succeed this time.

[symat]

@stoty , can you please take a look on these yetus checks? Maybe I missed something... it is my first PR in Phoenix, as far as I remember :)

error: @author — The patch appears to contain 8 @author tags which the community....

This was indeed an issue in the original patch, but was fixed in my second commit.

checkstyle — root: The patch generated 2 new + 117 unchanged

Here I see two remaining checkstyle failures on SortablePicklistItem.java. This class was part of my first commit, but later I realized that we don't need it and I deleted this class. Yet it appears in the latest checkstyle report for some reason.

[symat]

would it help if I would create a new PR with a single squashed commit? Or force-push a single squashed commit here to this PR?

[gjacoby126]

@symat - let's try squashing to a single commit and force-pushing. I don't see the author tags it's complaining about either.

[stoty]

Pushed to master and 5.1

[symat]

thank you @gjacoby126 and @stoty for the help!