BUG: Fixed Unicode decoding error in `Period.strftime` when a locale-specific directive is used
State: closed

- [x] closes #46319 , closes #46468
- [x] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [x] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.

Note: this was extracted from #46116 to decompose the big PR into reviewable bits. More to come soon

Labels:
name: Unicode
description: Unicode strings
name: Period
description: Period data type

Comments:
I have an issue: I can reproduce the bug locally but not on CI.
It seems that there is no CI runner as of today that uses non-utf8 encoded locale.

The issue can only be reproduced when the current locale encoding is not utf8:

![image](https://user-images.githubusercontent.com/3236794/158831195-ec9691e2-1674-4b70-bdb7-c1a3631fc2c8.png)

(zh_CN defaults encoding is gb2312)

What is the recommended way to proceed ?

 - fix the issue without having the CI reproduce it ? (not recommended...)
 - add a workflow in `posix.yml`
 - other ?

I'll try the second option for now (when github issues are fixed, it seems down for now)

gha runner was not triggered apparently :( re-trying
Unnfortunately it does not work :( 

> /usr/bin/bash: warning: setlocale: LC_ALL: cannot change locale (zh_CN.GB2312)

EDIT: retrying with zh_CN.gb2312
ok with lower case `zh_CN.gb2312` it is worse: the runner does not even start :(
Can anyone help me to define a runner configured with this locale+encoding ? (@jreback maybe ?) Otherwise I am not able to reproduce the issue on CI, which is a bit problematic to prevent regression once I push the fix.
I made progress: I was able to install the missing locale. However this leads to a segmentation fault !

https://github.com/pandas-dev/pandas/runs/5606041220?check_suite_focus=true#step:12:22

I will not try to resolve this in this PR.

Instead, my new attempt will be to install (but not activate) the `zh_CN` locale in the `zh_ZH.utf-8` worker. It will then only be activated in the test requiring it.

It worked  ! :)

https://github.com/pandas-dev/pandas/runs/5606395924?check_suite_focus=true#step:12:57

Now that the bug can be reproduced on CI, I will push the fix, probably monday.

All set - eventually I did it today :) 
Ready for review. Maybe @mroeschke or @jbrockmendel  ?
Good to go for another round of review.

As a side note, I saw that `util.pxd/get_c_string` (used in `hashtable_class_helper.pxi`) seems to also be encoding the input python string to char* using unicode and not the system locale (I think that instead to use the system locale we should use PyUnicode_EncodeLocale + PyByteArray_AsString). 
If this is only used for hashing it might not be a problem at all (even something good actually!), but maybe there are other consequences ? I let you check, this is off-topic for this PR

Strange issue on the numpy runner: https://github.com/pandas-dev/pandas/runs/5624496261?check_suite_focus=true#step:12:88
Not sure it is related, I'll just update the branch and run again to see if the issue is still here.
The issue disappeared. All fine!
lgtm. @mroeschke @jbrockmendel good here?
Are any of the functions added to util.pxd going to be used outside of period.pyx?  everything in util.pxd gets added to the source of _every_ C file cython generates (if we cimport util), and i don't know if that has any cost or gets fully optimized away
nitpick on the whatsnew message, don't know the CI config well enough to comment there but trust matt to be on top of it
Thanks @jbrockmendel !

> Are any of the functions added to util.pxd going to be used outside of period.pyx? everything in util.pxd gets added to the source of _every_ C file cython generates (if we cimport util), and i don't know if that has any cost or gets fully optimized away

Good point. Currently `pandas/_libs/tslibs/util.pxd` is 

### Imported in `pandas\_libs\tslibs`

 - Whole module (`cimport pandas._libs.tslibs.util as util` or `from pandas._libs.tslibs cimport util`) in : `offsets.pyx`, `period.pyx`, `nattype.pyx` and `timedeltas.pyx`
 - Specific symbols (`from pandas._libs.tslibs.util cimport <name>`) in : `np_datetime.pyx`, `conversion.pyx`, `offsets.pyx`, `parsing.pyx`, `timedeltas.pyx`, `timestamps.pyx`, `timezones.pyx`

Note that for `offsets.pyx` and `timedeltas.pyx`, both styles of import are used simultaneously.

### Imported in `pandas\_libs\`

It is only used in three files:

 - Specific symbols (`from pandas._libs.tslibs.util cimport <name>`) in `hashtable_class_helper.pxi`, `hashtable_class_helper.pxi.in`, `interval.pyx`, 

For the rest of its usage, it is entirely imported (`from pandas._libs.tslibs.util cimport *`) in the broader `pandas/_libs/util.pxd` lib (same name but broader scope). This broader `util` is imported as follows

 - Whole module (`cimport pandas._libs.util as util` or `from pandas._libs cimport util`) in `algos.pyx`, `groupby.pyx`, `hashtable.pyx`, `index.pyx`, `interval.pyx`, `lib.pyx`, `missing.pyx`, `parsers.pyx`
 - Specific symbols (`from pandas._libs.util cimport <name>`) in `parsers.pyx`, `algos.pyx`, `hashing.pyx`, `internals.pyx`, `lib.pyx`, `ops.pyx`, `reduction.pyx`, `testing.pyx`, `tslib.pyx`


### Discussion

For the PR at hand, I just added the symbols in the same place where `char_to_string` was sitting, as on of the new functions (`char_to_string_locale`) has exactly the same role. It also seems better for long-term maintenance that all such common utils are in the same place. 

However, your comment (and the above landscape) makes me think that if there is really a performance or size issue related to the util lib imports, it should be treated globally in a distinct PR. Do you think that replacing all "entire module" imports with specific symbol imports could make the trick ? If so I can propose a distinct PR for this. Maybe we should double-check with cython docs.

EDIT: I did not find much in cython docs. I guess that replacing all broad module cimports with specific symbol cimports would not hurt anyway, and might even possibly improve the code. So it could be a safe decision.


Note: I just replaced the whatsnew text (nitpick from @jbrockmendel ) and updated to master HEAD and the CI got all failing, so I guess this is not related :)
> Note: I just replaced the whatsnew text (nitpick from @jbrockmendel ) and updated to master HEAD and the CI got all failing, so I guess this is not related :)

 Unless it changed from last night, its an unrelated problem that will be fixed by https://github.com/pandas-dev/pandas/pull/46478/commits/14bb45d930bfc9880b2f43fb2f4d1e40a379b987

w/r/t util, I just meant that if the new functions added to util are only used in period.pyx, it may be better to just define them directly in period.pyx.  NBD.
I updated the branch just in case.
@mroeschke : I think that we are just missing your last review (@jbrockmendel and @jreback are ok).
I would be glad to have this merged so that I can move on to the next (probably 2) remaining parts of https://github.com/pandas-dev/pandas/pull/46116/
oh I see that there is some work going on on ci.I'll understand that this may cause delays in reviews, no worries
I updated the branch. It seems that the CI problems are solved, good job !
2 windows workers were taking too long and got cancelled, however the rest was successful. 
@mroeschke I see that you added comments, sorry I just see them now :( . Let me have a look.
@mroeschke I took into account your comment about getting rid of the fixture.
Doing so made me discover a nasty little bug #46595 . I added the fix in the PR.

There is just a single comment remaining, my guess is that this is a broader topic that cannot really be solved here.
The CI seems to be failing because of a pytest.xfail mark such as these ones

```
pytest.param(
                "01/10/2010 08:14 PM",
                "%m/%d/%Y %I:%M %p",
                Timestamp("2010-01-10 20:14"),
                marks=pytest.mark.xfail(
                    locale.getlocale()[0] in ("zh_CN", "it_IT"),
                    reason="fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8",
                ),
            ),
```

output:

> [XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8

It does not seem *directly* related to this PR, is this some ongoing work in parallel on the same locale topic ?
I'll maybe wait a couple days and update again, to see if something changes. 


This pull request is stale because it has been open for thirty days with no activity. Please [update](https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#updating-your-pull-request) and respond to this comment if you're still interested in working on this.
bot: please leave this open.
I updated again this PR to the latest branch, and the failures still remain. I am wondering if this could be related to the fix for #46595 (and not to the main contents of this PR). Therefore, I created a dedicated PR "just for this fix": https://github.com/pandas-dev/pandas/pull/47570

If it triggers the same bugs, this is probably the cause...
Now that https://github.com/pandas-dev/pandas/pull/47570 is merged, I updated this PR. Ready for a new round of review @jbrockmendel and @mroeschke 

EDIT: actually this seems to be failing on a specific runner (Numpy).

```
2022-07-12T08:18:14.6505018Z     def _box_func(self, x) -> Period | NaTType:
2022-07-12T08:18:14.6505328Z >       return Period._from_ordinal(ordinal=x, freq=self.freq)
2022-07-12T08:18:14.6505876Z E       TypeError: Argument 'ordinal' has incorrect type (expected int, got numpy.int64)
```

I'll investigate. If you have any hint about it, do not hesitate to share :)
I could not find any explanation, except maybe that the `period.pyx` was recompiled with different flags (more strict ? additional checks ?) on that "Numpy" worker, and the same worker usually maybe does not get to recompile this file (?)
Anyway, I'm trying to fix this by simply changing the type hint from `def _from_ordinal(cls, ordinal: int, freq)` to `def _from_ordinal(cls, ordinal: int64_t, freq)`. Let's see if it fixes everything.

@mroeschke @jbrockmendel if you have alternative ideas (maybe adding an explicit union type accepting ints and int64_t ?). I did not yet understand clearly the current typing policy concerning numbers from stdlib vs numpy
Oh I think I found it: `Period.__new__` was called with the wrong, inverted arguments. So strange that this was not making any test fail ?!

EDIT: 

 - no, this was not a bug, `_Period.__new__` was called not `Period.__new__`. Anyway using keyword args does not hurt so I let it there.
 - using `int64_t` instead of `int` in the signature does seem to reduce the number of failing tests on numpy-dev: before the change we have [`2340 failed`](https://pipelines.actions.githubusercontent.com/serviceHosts/e1fa798b-a3de-4f41-9248-cf5942f5881f/_apis/pipelines/1/runs/301131/signedlogcontent/7?urlExpires=2022-07-12T16%3A06%3A26.6576898Z&urlSigningMethod=HMACV1&urlSignature=ip33RPf66whKkVoYfQVYXY%2FAOX3E71e39j7dIaxm1tc%3D) and after [`30 failed`](https://pipelines.actions.githubusercontent.com/serviceHosts/e1fa798b-a3de-4f41-9248-cf5942f5881f/_apis/pipelines/1/runs/301198/signedlogcontent/7?urlExpires=2022-07-12T15%3A44%3A47.7190852Z&urlSigningMethod=HMACV1&urlSignature=bshlHMXymdwMBflHF9%2FC7TUIZxfMYPSTH8L8SdsxJkc%3D). I will now try to understand why the datetime arrays show as numpy arrays instead of pandas objects (the remaining issues it seems)
I am totally stuck: I do not understand how my modification in `period_format` could have an impact on `DatetimeArray` such as 
 https://github.com/pandas-dev/pandas/runs/7303444226?check_suite_focus=true#step:8:134

I added asserts on type to understand which line in the test is actually the culprit when running on numpy-dev