BUG: `sort_values` should have consistent behavior irrespective of the number of sort columns

[arpit-goel]

Pandas version checks

• I have checked that this issue has not already been reported.

• I have confirmed this bug exists on the latest version of pandas.

• I have confirmed this bug exists on the main branch of pandas.

Reproducible Example

import pandas as pd
import datetime
df = pd.DataFrame([["", 1], [datetime.date.today(), 2]], columns=['a', 'b']) # column a contains mixed data
df.sort_values(['a']) # raises error
df.sort_values(['a', 'b'])  # no error
df.sort_values(['a', 'a'])  # no error

Issue Description

When using sort_values on a data frame where at least one column contains mixed data, it should handle it gracefully or raise appropriate errors. It raises an error if a mixed-type column is used for sorting and succeeds if multiple columns are used, where one of them is a mixed-type column.

Expected Behavior

The behvior should be consistent irrespective of the number of columns being used for sorting.

Installed Versions

INSTALLED VERSIONS

commit : 0f43794
python : 3.9.16.final.0
python-bits : 64
OS : Darwin
OS-release : 22.5.0
Version : Darwin Kernel Version 22.5.0: Thu Jun 8 22:22:22 PDT 2023; root:xnu-8796.121.3~7/RELEASE_X86_64
machine : x86_64
processor : i386
byteorder : little
LC_ALL : en_US.UTF-8
LANG : en_US.UTF-8
LOCALE : en_US.UTF-8
pandas : 2.0.3
numpy : 1.24.3
pytz : 2022.1
dateutil : 2.8.2
setuptools : 65.5.1
pip : 22.0.4
Cython : None
pytest : None
hypothesis : None
sphinx : None
blosc : None
feather : None
xlsxwriter : 3.0.3
lxml.etree : 4.9.1
html5lib : None
pymysql : None
psycopg2 : 2.9.3
jinja2 : 3.0.1
IPython : 8.10.0
pandas_datareader: None
bs4 : 4.11.1
bottleneck : None
brotli : None
fastparquet : None
fsspec : 2022.5.0
gcsfs : None
matplotlib : 3.7.0
numba : None
numexpr : None
odfpy : None
openpyxl : 3.0.9
pandas_gbq : None
pyarrow : 14.0.1
pyreadstat : None
pyxlsb : None
s3fs : None
scipy : None
snappy : None
sqlalchemy : None
tables : None
tabulate : 0.8.9
xarray : None
xlrd : None
zstandard : None
tzdata : 2022.1
qtpy : None
pyqt5 : None

[remiBoudreau]

take

[rhshadrach]

Thanks for the report; looks like we take different paths depending on whether there is a single or multiple columns to sort by. I'd guess this was done for performance reasons. Further investigations and PRs to fix are welcome!

[remiBoudreau]

@rhshadrach Theres a few ways to patch this up but I'm concerned about some things.

Merging the 2 conditionals within the core dataframe class' sort_values method

pandas/pandas/core/frame.py

Lines 7155 to 7186 in f538741

	if len(by) > 1:
	keys = [self._get_label_or_level_values(x, axis=axis) for x in by]
	# need to rewrap columns in Series to apply key function
	if key is not None:
	# error: List comprehension has incompatible type List[Series];
	# expected List[ndarray]
	keys = [
	Series(k, name=name) # type: ignore[misc]
	for (k, name) in zip(keys, by)
	]
	indexer = lexsort_indexer(
	keys, orders=ascending, na_position=na_position, key=key
	)
	elif len(by):
	# len(by) == 1
	k = self._get_label_or_level_values(by[0], axis=axis)
	# need to rewrap column in Series to apply key function
	if key is not None:
	# error: Incompatible types in assignment (expression has type
	# "Series", variable has type "ndarray")
	k = Series(k, name=by[0]) # type: ignore[assignment]
	if isinstance(ascending, (tuple, list)):
	ascending = ascending[0]
	indexer = nargsort(
	k, kind=kind, ascending=ascending, na_position=na_position, key=key
	)

to

if len(by):
    keys = [self._get_label_or_level_values(x, axis=axis) for x in by]

    # need to rewrap columns in Series to apply key function
    if key is not None:
        # error: List comprehension has incompatible type List[Series];
        # expected List[ndarray]
        keys = [
            Series(k, name=name)  # type: ignore[misc]
            for (k, name) in zip(keys, by)
        ]

    indexer = lexsort_indexer(
        keys, orders=ascending, na_position=na_position, key=key
    )

ensures consistency by sorting using np.lexsort for all cases.

pandas/pandas/core/frame.py

Lines 7155 to 7191 in f538741

	if len(by) > 1:
	keys = [self._get_label_or_level_values(x, axis=axis) for x in by]
	# need to rewrap columns in Series to apply key function
	if key is not None:
	# error: List comprehension has incompatible type List[Series];
	# expected List[ndarray]
	keys = [
	Series(k, name=name) # type: ignore[misc]
	for (k, name) in zip(keys, by)
	]
	indexer = lexsort_indexer(
	keys, orders=ascending, na_position=na_position, key=key
	)
	elif len(by):
	# len(by) == 1
	k = self._get_label_or_level_values(by[0], axis=axis)
	# need to rewrap column in Series to apply key function
	if key is not None:
	# error: Incompatible types in assignment (expression has type
	# "Series", variable has type "ndarray")
	k = Series(k, name=by[0]) # type: ignore[assignment]
	if isinstance(ascending, (tuple, list)):
	ascending = ascending[0]
	indexer = nargsort(
	k, kind=kind, ascending=ascending, na_position=na_position, key=key
	)
	else:
	if inplace:
	return self._update_inplace(self)
	else:
	return self.copy(deep=None)

That being said, performance will suffer for single column case due to extra overhead of np.lexsort compared to current implementation using the more simple argsort.

Another approach is to modify the behaviour of the nargsort function in the sorting module to handle this specific case. In particular, changing

pandas/pandas/core/sorting.py

Lines 369 to 450 in f538741

	def nargsort(
	items: ArrayLike | Index | Series,
	kind: SortKind = "quicksort",
	ascending: bool = True,
	na_position: str = "last",
	key: Callable | None = None,
	mask: npt.NDArray[np.bool_] | None = None,
	) -> npt.NDArray[np.intp]:
	"""
	Intended to be a drop-in replacement for np.argsort which handles NaNs.
	Adds ascending, na_position, and key parameters.
	(GH #6399, #5231, #27237)
	Parameters
	----------
	items : np.ndarray, ExtensionArray, Index, or Series
	kind : {'quicksort', 'mergesort', 'heapsort', 'stable'}, default 'quicksort'
	ascending : bool, default True
	na_position : {'first', 'last'}, default 'last'
	key : Optional[Callable], default None
	mask : Optional[np.ndarray[bool]], default None
	Passed when called by ExtensionArray.argsort.
	Returns
	-------
	np.ndarray[np.intp]
	"""
	if key is not None:
	# see TestDataFrameSortKey, TestRangeIndex::test_sort_values_key
	items = ensure_key_mapped(items, key)
	return nargsort(
	items,
	kind=kind,
	ascending=ascending,
	na_position=na_position,
	key=None,
	mask=mask,
	)
	if isinstance(items, ABCRangeIndex):
	return items.argsort(ascending=ascending)
	elif not isinstance(items, ABCMultiIndex):
	items = extract_array(items)
	else:
	raise TypeError(
	"nargsort does not support MultiIndex. Use index.sort_values instead."
	)
	if mask is None:
	mask = np.asarray(isna(items))
	if not isinstance(items, np.ndarray):
	# i.e. ExtensionArray
	return items.argsort(
	ascending=ascending,
	kind=kind,
	na_position=na_position,
	)
	idx = np.arange(len(items))
	non_nans = items[~mask]
	non_nan_idx = idx[~mask]
	nan_idx = np.nonzero(mask)[0]
	if not ascending:
	non_nans = non_nans[::-1]
	non_nan_idx = non_nan_idx[::-1]
	indexer = non_nan_idx[non_nans.argsort(kind=kind)]
	if not ascending:
	indexer = indexer[::-1]
	# Finally, place the NaNs at the end or the beginning according to
	# na_position
	if na_position == "last":
	indexer = np.concatenate([indexer, nan_idx])
	elif na_position == "first":
	indexer = np.concatenate([nan_idx, indexer])
	else:
	raise ValueError(f"invalid na_position: {na_position}")
	return ensure_platform_int(indexer)

to

    try:
        indexer = non_nan_idx[non_nans.argsort(kind=kind)]
    except (TypeError, AttributeError):
        # Convert datetime objects to ordinal representations
        non_nans_ordinals = []
        for d in non_nans:
            if isinstance(d, datetime.date):
                non_nans_ordinals.append(d.toordinal())
            else:
                # Handle non-date values gracefully
                non_nans_ordinals.append(d)

        indexer = non_nan_idx[np.argsort(non_nans_ordinals, kind=kind)]

keeps the n_columns == 1 and > 1 situation have consistent sorting for the case where a , but introduces inconsistency for other cases. For example, if you add an additional row of all with every column as value 3 to the dataframe, the n_columns > 1 case fails while the n_columns == 1 case passes.

import pandas as pd
import datetime
df = pd.DataFrame([["", 1], [datetime.date.today(), 2], [3, 3]], columns=['a', 'b']) # column a contains mixed data
df.sort_values(['a']) # fixes this error
df.sort_values(['a', 'b'])  # new error
df.sort_values(['a', 'a'])  # new error

where the new error is

TypeError: 'values' is not ordered, please explicitly specify the categories order by passing in a categories argument.

I guess I'm unsure whether there is any benefit to ensure consistency if performance will take a hit... second solution is beneficial but only for mixed type columns involving datetime objects. I'm also thinking it might actually be better to error out in case there are inconsistencies in the format of entries in a column so that its caught and relayed to the user. anyone have any thoughts?

[arpit-goel]

@remiBoudreau My 2 cents would be on ensuring consistency, whether that's raising errors or successful sorting. Of course, performance is important but not as important as accuracy and correctness. Let's try and side ourselves with whether we want to allow mixed-type columns or not and then the decision will follow to either raise errors or fix sorting.

[rhshadrach]

@remiBoudreau - I think we should maintain the fastpath for a single by - this is a very common case. That leaves either improving this path to handle this case, or having it raise in both cases. If we are to improve the fastpath to handle this case, it shouldn't be by using try...except.... That can hurt performance, hide bugs, and make things harder to debug.

If we want to make it so both paths raise here, we'd need to deprecate the current behavior for multiple by values. For this, a first step is in deciding what we want and don't want to support. For example, I know groupby has specific logic for handling sorting with integers, strings, and NA values mixed in.