DEPR: disallow tznaive datetimes when indexing tzaware datetimeindex

[jbrockmendel]

• closes #xxxx
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

cc @mroeschke

[jreback]

IIRC there is an issue about this, and we had quite some discussion. I am +1 on actually doing this, but we might need to deprecate first. cc @jorisvandenbossche @TomAugspurger

[mroeschke]

+1 on doing this too. It would be great to do this for tz-naive datelike-strings as well.

[jbrockmendel]

IIRC there is an issue about this, and we had quite some discussion

would be great to do this for tz-naive datelike-strings as well.

The discussion 2ish years ago ended up deciding to special-case naive strings because a) user convenience and b) there's not a nice way to specify a tz in a string for e.g. "US/Eastern". IIRC the consensus was pretty specifically strings only, and the fact that we currently allow mismatched datetime/timestamp objects is the bug.

xref #18435, #17920, #18376, #24076

[jorisvandenbossche]

A more recent discussion about this is in #30994 (I think this PR is meant to address that issue?)

IIRC the consensus was pretty specifically strings only,

Yes, I think that is correct.
But that doesn't mean the current behaviour is a bug. It's maybe accidental, but nonetheless long-standing behaviour that we have been discussing for a long time (see all the elaborate discussions you are referencing), so it's clearly not that uncontroversial. So I agree with @jreback that we should do this with a deprecation cycle.
But to be clear: fully on board with the proposed change in behaviour.

the fact that we currently allow mismatched datetime/timestamp objects is the bug.

What do you mean exactly with "mismatched datetime/timestamp objects" (just the fact of tz-naive vs tz-aware)?

[jbrockmendel]

@jorisvandenbossche so i have a way forward, is your suggestion that the two self._data._check_compatible_with calls this PR adds should be changed to something like

try:
    self._data._check_compatible_with(other)
except TypeError:
    warnings.warn("deprecated...", FutureWarning)

If so, is everyone else on board with that?

FWIW i think this behavior is sufficiently inconsistent we should call it a bugfix and be done with it, but I'm OK with deprecating if there's a consensus.

[jreback]

@jorisvandenbossche so i have a way forward, is your suggestion that the two self._data._check_compatible_with calls this PR adds should be changed to something like

try:
    self._data._check_compatible_with(other)
except TypeError:
    warnings.warn("deprecated...", FutureWarning)

If so, is everyone else on board with that?

FWIW i think this behavior is sufficiently inconsistent we should call it a bugfix and be done with it, but I'm OK with deprecating if there's a consensus.

+1 on this.

note we are deprecating for strings AND timestamps correct? (I think we should)

[jbrockmendel]

note we are deprecating for strings AND timestamps correct? (I think we should)

the discussion so far has been about changing/deprecating the incorrect timestamp behavior, not about undoing the special-casing for strings

[jbrockmendel]

updated as deprecation (though i still think this should be considered a bugfix)

[jreback]

lgtm, just resolved the conflict.

[jreback]

lgtm merge on green.

[phofl]

@jbrockmendel Should this be checked for insert too? Currently we are raising a FutureWarning for allow_duplicates=False because we run into if not allow_duplicates and column in self.columns:.

If this should raise I would either change the order of the calls here or explicitly check for the mismatch. If you remove this line, no warning is raised

[jbrockmendel]

So there's a case where frame.columns is tzaware and the user is doing frame.insert(int, tznaive_datetime, value)?

The __contains__ behavior is going to change for this user in once the deprecation is enforced, so they should see the warning right?

(or maybe a more specific warning?)