Parse start/end only if Index of type Datetime
State: closed

Many of the existing tests are already failing if compiling with [sanitizers](https://github.com/google/sanitizers):
> runtime error: signed integer overflow: 253370764800000000 * 1000 cannot be represented in type 'long'

One pretty common use case when this happens, is when using a `TimedeltaIndex`, and passing a string representing a time delta to index it (e.g. '12 hours'). The existing code still tries to perform a Datetime specific check which leads to a cast attempt `Timestamp('12 hours')`, which in normal conditions is caught as a python exception by the `try: except:` and ignored (hence, the casting is ignored). However, it still leads to an unnecessary overflow happening in between which is flagged as an error if compiling with sanitizers.

As far as I can tell, there is not reason to run this Datetime specific check in the first place, unless we are dealing with an indexer for Datetime objects, hence the proposed change.


Labels:
name: Stale

Comments:
Hello @alvarosg! Thanks for opening this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:

* In the file [`pandas/core/indexes/base.py`](https://github.com/pandas-dev/pandas/blob/4256ddc95ce08ae7e95016fbe6ecf0cefd94c12a/pandas/core/indexes/base.py):

> [Line 6667:13](https://github.com/pandas-dev/pandas/blob/4256ddc95ce08ae7e95016fbe6ecf0cefd94c12a/pandas/core/indexes/base.py#L6667): [E129](https://duckduckgo.com/?q=pep8%20E129) visually indented line with same indent as next logical line


can you show something that reproduces what is the problem
I think the problem is that the arithmetic in https://github.com/pandas-dev/pandas/blob/e7a0cf3ba098c0074a4462522da5c7fdca804c78/pandas/_libs/tslibs/src/datetime/np_datetime.c#L472-L479  can overflow when the string isn't a valid date, which you can trigger for example with `pd.Timestamp("12 hours")`. The overflow has undefined behaviour (though this may not always manifest in a noticeable way).

The right thing to do would be to only perform the arithmetic if the input was valid.

An example of the error I saw (under ASAN) was:

```runtime error: signed integer overflow: -62135553600000000 * 1000 cannot be represented in type 'long'```

I assume the large negative number arose from a previous failed parse.
ok pls add a test that triggers this
Do you have a test setup that runs with ASAN? We'd need that to expose the undefined behaviour. (Undefined behavior means that we can't have a well-defined in-band test for this.)
i have no idea what ASAN is

this would be a test in the test suite 
ASAN is the address sanitizer, i.e. build with `-fsanitize=address` (e.g. see https://en.wikipedia.org/wiki/AddressSanitizer). If you're not running that, it'll be difficult to demonstrate that a program is causing undefined behaviour. Unit tests can really only effectively talk about things that behave _well_; only for well-behaving code does it make sense to ask if it's correct. To reliably show that code has undefined behaviour we need some form or runtime sanitizer, e.g. ASAN or Valgrind or some such.
@alvarosg if the expl you give above is true then the current code should raise an error on this overflow. a test should be able to replicate that. 
Thanks for your reply:

> if the expl you give above is true then the current code should raise an error on this overflow. a test should be able to replicate that.

Note when you execute: `pd.Timestamp('12 hours')` you do get an exception, and this is probably expected and probably already tested:
> OutOfBoundsDatetime: Out of bounds nanosecond timestamp: 1-01-01 12:00:00

The problem, is that even before the exception is raises, there is already an overflow. By default this cannot be flagged by any tests, in the same way that if you do:
```
import numpy as np
np.array(5, dtype=np.int32) + np.array(2 ** 31, dtype=np.int32). # -2147483643
```
there is an overflow, but not exception is raised, unless you compile with `-fsanitize=address`.

A similar behavior to this occurs when running `pd.Timestamp('12 hours')` in the code linked above by @tkoeppe, and this occurs *before* the pythio exception is raised.

The fact that this overflow is not explicitly avoided before computing the problematic arithmetic, not only produces undefined overflow behavior, but it means that the existing test actually fails if you compile them with a sanitizer that checks for overflows, because the overflow does happen even before the exception is raised, so the test built with the address sanitizer `-fsanitize=address` fails at that point, even before raising the exception, and even if the whole purpose of the test was to catch the expected exception.

Now, I have tried running all of the existing tests in the library with the address sanitizer, and up to 50 of the test modules fail, so this seems to be a much bigger problem that what I am trying to fix in this CL. I guess it is not technically a requirement of a library tests to pass with the sanitizer. The problem is when other code that uses pandas as a dependency cannot even be tested with the sanitizer. In our case this perfectly valid code:

```
data = np.arange(5) * np.timedelta64(6, "h")
delta_index = pd.TimedeltaIndex(data=data, name="time")
delta_index.slice_locs("12 hours", "1 day")
```
fails if compiled with the sanitizer, and the reason it fails is because it relies on catching the exception raised when doing `pd.Timestamp('12 hours')` which not only raises a python exception, but produces an overflow before that.



oh if u can add the compile flag then this becomes much simpler

pls add that in a separate PR and then can work on fixing those issues

i would rather be explicit about this 
@jreback: "the current code should raise an error on this overflow" -- I don't think this is true: C is not a language that can prove correctness of a program within itself. In other words, a program can have an error that the program cannot demonstrate within its own semantics. That's why you need external tools such as runtime sanitizers to spot these errors. C calls this "undefined behaviour", meaning that a program that executes the erroneous construction is not governed by the rules of the C language.

For a simple example, consider the function `int succ(int n) { return n + 1; }`. Its implementation clearly imposes a precondition, namely that it not be called with an argument that is too large (specifically, with `INT_MAX`). But you cannot write C code that shows that such a call is an error. No "error is raised" here; it is simply the language rules that say that this program is only a valid C program with well-defined semantics if you don't make such a call.

The short, high-level way to state this is that "you cannot express expectations on behaviour out of contract".

What we are pointing out is that currently it is possible for the Python code to invoke the C code out of contract. A sanitizer will easily show this (and I recommend, and I believe it is modern industry best practice, that large and important projects run sanitizing versions of their tests). But we cannot show that _within C_, just like we cannot test for the erroneous call of `succ` in my above example.

There are at least two possible fixes: 1) add a check in the Python layer to raise an out of range error purely in Python before invoking the native extension, or 2) add a range check to the native code. Another possibility is to change the implementation to support larger values.
> oh if u can add the compile flag then this becomes much simpler
I tried mentioning that in https://github.com/pandas-dev/pandas/pull/46385#issuecomment-1069369496 :-)
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.
I will close this for now. We may try to make a bigger case to make sanitizers compilation not break in a more coordinated effort in there future. Thanks!