PERF: fix merging on datetimelike columns to not use object-dtype factorizer #53231

jorisvandenbossche · 2023-05-15T08:20:31Z

Running the added benchmark on current main gives:

[100.00%] ··· join_merge.MergeDatetime.time_merge           ok
[100.00%] ··· ============== ============= =================
              --                            tz              
              -------------- -------------------------------
                  units           None      Europe/Brussels 
              ============== ============= =================
               ('ns', 'ns')    16.1±0.3ms      16.1±0.3ms   
               ('ms', 'ms')   20.7±0.07ms      21.0±0.2ms   
               ('ns', 'ms')     167±2ms        16.9±0.4ms   
              ============== ============= =================

versus on this branch:

[100.00%] ··· ============== ============= =================
              --                            tz              
              -------------- -------------------------------
                  units           None      Europe/Brussels 
              ============== ============= =================
               ('ns', 'ns')   6.33±0.09ms      6.44±0.1ms   
               ('ms', 'ms')    6.95±0.2ms      7.06±0.1ms   
               ('ns', 'ms')   6.90±0.08ms     6.91±0.09ms   
              ============== ============= =================

(and running the standard ns/ns example on pandas 1.5, it also gave around 7ms, so this should restore the performance of 1.5)

Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

…torizer

jorisvandenbossche · 2023-05-15T08:22:37Z

pandas/core/reshape/merge.py

+    if needs_i8_conversion(lk.dtype) and lk.dtype == rk.dtype:
+        # GH#23917 TODO: Needs tests for non-matching dtypes
+        # GH#23917 TODO: needs tests for case where lk is integer-dtype
+        #  and rk is datetime-dtype
+        lk = np.asarray(lk, dtype=np.int64)
+        rk = np.asarray(rk, dtype=np.int64)


This part was (accidentally?) removed in #49876. So there won't be any tests failing from leaving this out (because falling back to the object factorizer also gives correct results), but it is necessary to ensure we use the faster factorizer for datetimelikes

i expect that the relevant cases should go through the elif isinstance(lk, ExtensionArray) and lk.dtype == rk.dtype: branch above?

Not fully:

It did go through there before but only for tz-naive data (and same unit), now we already convert to a numpy array in the if isinstance(lk.dtype, DatetimeTZDtype) ... block by getting the _ndarray of lk and rk (but we could not do that there, and let it go through the block that calls _values_for_factorize)

even when going through this block, the DatetimeArray._values_for_factorize returns the _ndarray, which is the M8 numpy array, not an integer numpy array. So we still need this needs_i8_conversion.
Now, we should maybe change _values_for_factorize? (but that's probably more for something targetting 2.1, and didn't check the other places where _values_for_factorize is being used)

makes sense, thanks.

jbrockmendel · 2023-05-15T15:58:22Z

pandas/core/reshape/merge.py

@@ -2355,12 +2355,14 @@ def _factorize_keys(
    rk = extract_array(rk, extract_numpy=True, extract_range=True)
    # TODO: if either is a RangeIndex, we can likely factorize more efficiently?

-    if isinstance(lk.dtype, DatetimeTZDtype) and isinstance(rk.dtype, DatetimeTZDtype):
+    if (isinstance(lk, DatetimeArray) and isinstance(rk, DatetimeArray)) and (
+        lk._is_tzawareness_compat(rk)


is checking this way more performant?

No idea, but that was also not the reason for using this: what we want to check here is that we have either two DatetimeTZDtypes, or two np.dtype("M8").

So initially I expanded the original if isinstance(lk.dtype, DatetimeTZDtype) and isinstance(rk.dtype, DatetimeTZDtype) with something like or (isinstance(lk.dtype, np.dtype) and lk.dtype.kind == "M" and isinstance(rk.dtype, np.dtype) and rk.dtype.kind == "M"). But this got a bit long, and so this seemed simpler (and since we are calling a specific DatetimeArray method in the if-block, it also make sense code-logic-wise to check for it).

(the performance benefit of the PR comes from avoiding using the ObjectFactorizer)

OK. I prefer the status quo here largely bc it avoids introducing a new method. Not a huge deal though.

BTW new optimized check for isinstance(lk.dtype, np.dtype) and lk.dtype.kind == "M" is lib.is_np_dtype(lk.dtype, "M")

OK, with is_np_dtype that can be shorter, so reverted back to the original dtype checks (and now with the added dtype check for numpy dtypes), avoiding the new method

jorisvandenbossche · 2023-05-31T08:04:33Z

@jbrockmendel anything else here, or good to go?

jbrockmendel

LGTM

mroeschke · 2023-05-31T17:12:37Z

Thanks @jorisvandenbossche

…ns to not use object-dtype factorizer

… columns to not use object-dtype factorizer) (#53471) * Backport PR #53231: PERF: fix merging on datetimelike columns to not use object-dtype factorizer * Update pandas/core/reshape/merge.py * Check npdtype --------- Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

…torizer (pandas-dev#53231) * PERF: fix merging on datetimelike columns to not use object-dtype factorizer * don't try to match resos for Arrow extension array * fix typing * try fix typing * update dtypes check * add whatsnew

PERF: fix merging on datetimelike columns to not use object-dtype fac…

d5c7604

…torizer

jorisvandenbossche added Timeseries Performance Memory or execution speed performance Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels May 15, 2023

jorisvandenbossche added this to the 2.0.2 milestone May 15, 2023

jorisvandenbossche commented May 15, 2023

View reviewed changes

don't try to match resos for Arrow extension array

971f267

jorisvandenbossche mentioned this pull request May 15, 2023

FIX preserve dtype with datetime columns of different resolution when merging #53213

Merged

5 tasks

jorisvandenbossche added 2 commits May 15, 2023 14:04

fix typing

caec63a

try fix typing

35a650d

jbrockmendel reviewed May 15, 2023

View reviewed changes

jorisvandenbossche added 2 commits May 25, 2023 17:04

Merge remote-tracking branch 'upstream/main' into perf-merge-datetime

47e6bd7

update dtypes check

52c768b

datapythonista modified the milestones: 2.0.2, 2.0.3 May 26, 2023

Merge remote-tracking branch 'upstream/main' into perf-merge-datetime

fafb61d

add whatsnew

d46d7c4

jbrockmendel approved these changes May 31, 2023

View reviewed changes

mroeschke approved these changes May 31, 2023

View reviewed changes

mroeschke merged commit 4c3af48 into pandas-dev:main May 31, 2023
34 checks passed

meeseeksmachine mentioned this pull request May 31, 2023

Backport PR #53231 on branch 2.0.x (PERF: fix merging on datetimelike columns to not use object-dtype factorizer) #53471

Merged

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request May 31, 2023

Backport PR pandas-dev#53231: PERF: fix merging on datetimelike colum…

6ccb4c6

…ns to not use object-dtype factorizer

jorisvandenbossche deleted the perf-merge-datetime branch May 31, 2023 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PERF: fix merging on datetimelike columns to not use object-dtype factorizer #53231

PERF: fix merging on datetimelike columns to not use object-dtype factorizer #53231

jorisvandenbossche commented May 15, 2023

jorisvandenbossche May 15, 2023

jbrockmendel May 25, 2023

jorisvandenbossche May 26, 2023 •

edited

jbrockmendel May 26, 2023

jbrockmendel May 15, 2023

jorisvandenbossche May 16, 2023 •

edited

jbrockmendel May 16, 2023

jorisvandenbossche May 25, 2023

jorisvandenbossche commented May 31, 2023

jbrockmendel left a comment

mroeschke commented May 31, 2023

PERF: fix merging on datetimelike columns to not use object-dtype factorizer #53231

PERF: fix merging on datetimelike columns to not use object-dtype factorizer #53231

Conversation

jorisvandenbossche commented May 15, 2023

jorisvandenbossche May 15, 2023

Choose a reason for hiding this comment

jbrockmendel May 25, 2023

Choose a reason for hiding this comment

jorisvandenbossche May 26, 2023 • edited

Choose a reason for hiding this comment

jbrockmendel May 26, 2023

Choose a reason for hiding this comment

jbrockmendel May 15, 2023

Choose a reason for hiding this comment

jorisvandenbossche May 16, 2023 • edited

Choose a reason for hiding this comment

jbrockmendel May 16, 2023

Choose a reason for hiding this comment

jorisvandenbossche May 25, 2023

Choose a reason for hiding this comment

jorisvandenbossche commented May 31, 2023

jbrockmendel left a comment

Choose a reason for hiding this comment

mroeschke commented May 31, 2023

jorisvandenbossche May 26, 2023 •

edited

jorisvandenbossche May 16, 2023 •

edited