[
https://issues.apache.org/jira/browse/CALCITE-6580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17882211#comment-17882211
]
Julian Hyde edited comment on CALCITE-6580 at 9/17/24 12:43 AM:
----------------------------------------------------------------
I have reviewed, and it looks good. I appreciate the effort to make this code
really clean.
I have revised your commit to add {{Locale.setDefault}} to the list of
forbidden APIs, and changed the two remaining legitimate uses to use a new
method {{Unsafe.setDefaultLocale(Locale)}}.
Regarding adding the {{Locale}} parameter to {{toCharPg}} (and similar
functions). You could instead have made it a field in {{class
DateFormatFunction}}. In CALCITE-5914, I made the assumption "The class must
have a public zero-args constructor." but that assumption can be lifted. At
least it should look for a constructor with a "DataContext root" parameter.
In {{toCharPg}} (and similar functions) you could cache the formatter. I have
logged CALCITE-6585 for this. (Not urgent, but will improve runtime efficiency
of the TO_CHAR function.)
was (Author: julianhyde):
I have reviewed, and it looks good. I appreciate the effort to make this code
really clean.
I have revised your commit to add {{Locale.setDefault}} to the list of
forbidden APIs, and changed the two remaining legitimate uses to use a new
method {{Unsafe.setDefaultLocale(Locale)}}.
Regarding adding the {{Locale}} parameter to {{toCharPg}} (and similar
functions). You could instead have made it a field in {{class
DateFormatFunction}}. In CALCITE-5914, I made the assumption "The class must
have a public zero-args constructor." but that assumption can be lifted. At
least it should look for a constructor with a "DataContext root" parameter.
In {{toCharPg}} (and similar functions) you could cache the formatter. I have
logged CALCITE-6585 for this.
> Remove Locale.setDefault
> ------------------------
>
> Key: CALCITE-6580
> URL: https://issues.apache.org/jira/browse/CALCITE-6580
> Project: Calcite
> Issue Type: Improvement
> Reporter: Julian Hyde
> Priority: Major
> Labels: pull-request-available
> Fix For: 1.38.0
>
>
> Remove all calls to {{Locale.setDefault}} in tests and production code.
> That function does not operate on the current thread; it affects all threads
> in the JVM. As such, it may affect other tests running at the same time, and
> affect other statements running at the same time.
> I hope, and believe, that the production code does not depend on
> {{Locale.getDefault}}. But let's make sure by removing {{setDefault}} from
> all tests.
> Add {{Locale.setDefault}} to
> [forbidden-apis|https://github.com/apache/calcite/blob/main/src/main/config/forbidden-apis/signatures.txt].
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
