zabetak commented on code in PR #3113:
URL: https://github.com/apache/calcite/pull/3113#discussion_r1193497546
##########
core/src/main/java/org/apache/calcite/sql/parser/SqlParserUtil.java:
##########
@@ -738,17 +741,7 @@ public static ParsedCollation parseCollation(String in) {
}
Charset charset = SqlUtil.getCharset(charsetStr);
- String[] localeParts = localeStr.split("_");
- Locale locale;
- if (1 == localeParts.length) {
- locale = new Locale(localeParts[0]);
- } else if (2 == localeParts.length) {
- locale = new Locale(localeParts[0], localeParts[1]);
- } else if (3 == localeParts.length) {
- locale = new Locale(localeParts[0], localeParts[1], localeParts[2]);
- } else {
- throw RESOURCE.illegalLocaleFormat(localeStr).ex();
Review Comment:
It seems that after these changes we are never going to throw the
`illegalLocaleFormat` exception. Are we going to throw another exception? Are
we going to fallback silently to something else?
If we cannot keep the old behavior then we should document the breaking
change in `history.md` file. Also if we never throw
`RESOURCE.illegalLocaleFormat` then we maybe we should also drop the respective
(dead) code.
##########
core/src/test/java/org/apache/calcite/util/UtilTest.java:
##########
@@ -914,7 +914,7 @@ private List<Integer> makeConsList(int start, int end) {
}
// Example locale names in Locale.toString() javadoc.
String[] localeNames = {
- "en", "de_DE", "_GB", "en_US_WIN", "de__POSIX", "fr__MAC"
+ "en", "de_DE", "fr_CA"
Review Comment:
The change implies that we no longer support certain patterns when parsing
locales so I think we should mention the change in behavior in `history.md`.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
