Remove Encoding of values in char** For Labels
State: closed

- [X] closes #20500

In reviewing this module there is a shared function for object keys and values which encodes objects into a separate buffer and subsequently indexes off of that. Instead of encoding values in a buffer I've updated that function to be a char ** pointing to string representations of the labels (or index / columns, rather). 

This is arguably a pre-cursor to:

  1. #19486 to disentangle date time functions from JSON
  2. #12004 to add indent support (tried this previously but vendoring ujson updates but didn't work because of this limitation)
  3. #27164 because the various formats may need column / index labels at different points in time, so encoding up front makes that very difficult to reuse

The only downside here I haven't been able to figure out is how to deal with date formatting. Right now all labels are written as epochs. I'm sure there is a way to handle but I wasn't clear on what the best way to convert arbitrary input (i.e. object or datetime dtypes) into ISO formats by element where applicable.

cc @jbrockmendel in case you have insight on that

Labels:
name: IO JSON
description: read_json, to_json, json_normalize
name: Clean

Comments:
can you give an example of the datetime inputs that are trouble?
Sure. I'm not sure how to best handle this:

```python
>>> df = pd.DataFrame([[1]], columns=[pd.Timestamp("today")])
>>> df.to_json(date_format="iso")
```

While also supporting:

```python
>>> df.columns = df.columns.astype(object)
>>> df.to_json(date_format="iso")
```

In a concise manner. AFAICT in the first example inspecting the object during iteration with `PyDateTime_Check` doesn't recognize it as a datetime whereas when passed in as an object it does
Right now the code handles neither and simply provides out the default str, which I guess is epoch for both cases
> AFAICT in the first example inspecting the object during iteration with PyDateTime_Check doesn't recognize it as a datetime whereas when passed in as an object it does

Off the top of my head, I'd guess that in the non-object case the array you're iterating over is a `ndarray[datetime64[ns]]` and not a DatetimeArray/DatetimeIndex.  As a result, the objects you get when iterating are datetime64 objects, which do not subclass datetime.  The check for that would be `PyObject_TypeCheck(obj, &PyDatetimeArrType_Type)` (at least that's how we do it in `tslibs.util`).
Thanks! Sounds in the direction I’ll give that a look

Sent from my iPhone

> On Jul 26, 2019, at 7:00 PM, jbrockmendel <notifications@github.com> wrote:
> 
> AFAICT in the first example inspecting the object during iteration with PyDateTime_Check doesn't recognize it as a datetime whereas when passed in as an object it does
> 
> Off the top of my head, I'd guess that in the non-object case the array you're iterating over is a ndarray[datetime64[ns]] and not a DatetimeArray/DatetimeIndex. As a result, the objects you get when iterating are datetime64 objects, which do not subclass datetime. The check for that would be PyObject_TypeCheck(obj, &PyDatetimeArrType_Type) (at least that's how we do it in tslibs.util).
> 
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub, or mute the thread.

Sorry should clarify - so if the columns are all datetimes it appears that `PyTypeNum_ISDATETIME(type_num)` is true (this was in the pre-existing code as well). Otherwise if mixed objects then `PyDateTime_Check` and `PyDate_Check` would work.

Do we have one function that can arbitrary convert objects from either as appropriate to an isoformat that you know of?
> Do we have one function that can arbitrary convert objects from either as appropriate to an isoformat that you know of?

In python/cython I would probably just use `Timestamp(obj).isoformat()`.  Not sure how big a hassle that would be to do in the c file
Hello @WillAyd! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:



There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-08-23 13:09:40 UTC
I think this is reviewable again. This is a little complicated so to summarize this change it would remove custom label encoding that we do in objToJSON. This theoretically would make the code easier to reason about and opens up the possibility to easily add #12004 indent support through work that is identical or similar to #26457 

A few caveats:
 - This change only affects *labels* which depending on the format could be any combination of the index and column values, if any at all
 - This causes performance regressions dealing with datetimelike objects as *labels only*. Benchmarks and reasoning are showing in Appendix A
 - Previous behavior would recursively encode labels. The plus side of this was that how items appear as keys would use the same logic was what appeared as a value for JSON objects. The downside is that this fights the JSON spec and ujson, as object keys by definition MUST be strings. It is also non-trivial to produce invalid JSON this way (see appendix B)
 - While the ultimate goal is to simplify things, this does introduce slightly different behavior for labels versus the actual data. For example, this change partially solves #15137 by appropriately handling Timedelta data in the labels, but doesn't work for Timedeltas appearing in the data. Ideally how Python objects are handled would be consistent regardless of location, but I've left this inconsistency for now because to truly make it work and resolve all performance implications might take a while

I feel like (save a few minor cleanups) it would be easiest to merge this in the current state and resolve label / data inconsistencies in follow ups, since to do that all at once would be a huge diff and tough to tackle incrementally. Open to suggestions on how to handle though.

## Appendix A
Here are all of the performance regressions. The reason for these is that when detecting a datetime-like array the new label encoding will create Python Timestamp objects for each entry, so that it can appropriately build an isoformat or scale the units from there. 

A vectorized solution would help a lot here and wouldn't be unreasonable to implement. I don't think this will be a long term / architectural problem just needs focus and addressing in follow up

```sh
       before           after         ratio
     [35810733]       [104d2f27]
     <remove-buf-writes~3^2>       <remove-buf-writes>
+        136±30ms        462±100ms     3.39  io.json.ToJSON.time_to_json('index', 'df_int_floats')
+         139±6ms        465±100ms     3.34  io.json.ToJSON.time_to_json('index', 'df_int_float_str')
+        125±30ms         365±80ms     2.93  io.json.ToJSON.time_to_json('index', 'df_date_idx')
+        131±10ms        357±100ms     2.72  io.json.ToJSON.time_to_json('index', 'df_td_int_ts')
+        117±20ms         309±40ms     2.64  io.json.ToJSON.time_to_json('columns', 'df_int_float_str')
+        128±20ms          301±6ms     2.36  io.json.ToJSON.time_to_json('columns', 'df_int_floats')
+        130±20ms         295±40ms     2.27  io.json.ToJSON.time_to_json('columns', 'df_date_idx')
+        150±50ms         301±20ms     2.01  io.json.ToJSON.time_to_json('columns', 'df_td_int_ts')
+         126±5ms         240±10ms     1.91  io.json.ToJSON.time_to_json_wide('records', 'df_date_idx')
+        194±10ms         323±40ms     1.67  io.json.ToJSON.time_to_json_wide('records', 'df_td_int_ts')
+         196±4ms         288±50ms     1.47  io.json.ToJSON.time_to_json_wide('records', 'df_int_float_str')
+         191±2ms         233±20ms     1.22  io.json.ToJSON.time_to_json_wide('columns', 'df_int_float_str')
+         125±1ms         149±10ms     1.19  io.json.ToJSONLines.time_floats_with_int_idex_lines
```

## Appendix B
As mentioned we would previously encode key values for JSON objects. This is pretty easy to break as shown in the example below (note the invalid string that this produces without error):

```python
In [15]: class Foo:
    ...:
    ...:     def toDict(self):
    ...:         return {"a": "b"}
    ...:
    ...: df = pd.DataFrame([[1]], columns=[Foo()])
    ...: print(df.to_json())
{"{"a":"b"}":{"0":1}}
```

This PR instead handles datetimelike objects by converting them to either epoch or iso formats and stringifying from there. All other objects are simply stringified. 

This has the added bonus of making tuples round-trippable albeit with data loss. To illustrate here is current master behavior:

```python
In [18]: pd.read_json(pd.DataFrame([[1]], index=[('a', 'b')]).to_json())
ValueError: No ':' found when decoding object value
```

As opposed to on this branch:
```python
In [5]: pd.read_json(pd.DataFrame([[1]], index=[('a', 'b')]).to_json())
Out[5]:
            0
('a', 'b')  1
```

So again some data loss but at the very least does not produce invalid JSON
How much of the difficulty here could be avoided by working in cython rather than C?  Moving the whole file to cython would be too much, but supposedly it is possible to make cython export a .h file so it can be used in a c file.  If I could figure out how to make this work, would that make things easier here?
Good question. I don't think we would want to move the entire extension to Cython since if done correctly this should theoretically be a very small extension that dictates key / value pairs for pandas objects. 

Right now there is too much custom logic here though (ex: date time handling) which if decoupled should be moved somewhere else for shared C functions (whether Cython or hand coded), so the majority can be removed but I don't consider entire replacement a goal
Actually this could close #20500 as well. I can add a test and whatsnew for that (though again wouldn’t be round trippable)
Any objections to merging this? As is this intentionally causes a regression when writing out a DTI but I think resolvable in a follow up. This refactor should also enable some of the items linked above.

I'm catching a flight back to the US tomorrow so should have spare time to focus on follow ups. Worst case I think could revert this before 1.0.0 if those never come to fruition
what's the DTI regression?
Takes 1-4x as long to write out index / columns containing 100k date times. Actual benchmarks are shown in Appendix A here https://github.com/pandas-dev/pandas/pull/27618#issuecomment-519568589

The reason is that this change constructs a Timestamp object during serialization for each label, whether or not that label is stuck in an object array or a DTI. 

A more performant approach would be to check for a DTI and dispatch to a vectorized stringification for the labels, which I think would just be better done in a follow up
Seems totally reasonable.
Sounds good. Merging for now will formalize follow ups over next few days. Worst case can always revert