chore: Remove unreachable hash check

[dangotbanned]

What type of PR is this? (check all applicable)

• 💾 Refactor
• ✨ Feature
• 🐛 Bug Fix
• 🔧 Optimization
• 📝 Documentation
• ✅ Test
• 🐳 Other

Related issues

• chore: Remove pandas-like specific check_column_names_are_unique util #2749 (comment)
• chore: Simplify PandasLikeDataFrame|DaskLazyFrame.join method #2511 (comment)

Checklist

• Code follows style guide (ruff)
• Tests added
• Documented the changes

If you have comments or can explain your changes, please do so below

Follow-up to #2749 (comment)

Could you give an example of how we'd get columns like that in the places the function is currently used?

import pandas as pd

>>> pd.DataFrame([[1, 2, 3], [4, 5, 6]], columns=([1], [2], [3]))
TypeError: unhashable type: 'list'

AFAIK, pandas only supports Hashable column "names" - so I'm a little confused 🤔

We're checking an existing NativeFrame.columns - so I would have thought that condition is not reachable

[MarcoGorelli]

ah we ended up removing it in https://github.com/narwhals-dev/narwhals/pull/2011/files

[MarcoGorelli]

Doesn't hurt to keep it, as per the linked test case? Like this it gives a clearer error message for malformed pandas dfs?

[dangotbanned]

Doesn't hurt to keep it, as per the linked test case? Like this it gives a clearer error message for malformed pandas dfs?

Sorry I'm not sure I understand.

You linked a test that was removed 4 months ago that has the comment:

def test_from_non_hashable_column_name() -> None:
   # This is technically super-illegal
    # BUT, it shows up in a scikit-learn test, so...

If that test is important - we should add it back?

Personally, I can't see why we should add error handling for a test fails in scikit-learn.
Maybe I could see this as important if we ran scikit-learn's test suite or if we had a test ourselves

As-is, we just have a check that's added overhead on every .with_native 🤔

[MarcoGorelli]

I doubt the overhead is noticeable, a comment with a link to the pandas issue should be fine

[FBruzzesi]

@MarcoGorelli I am also having a hard time to figure out how this can be used in practice. I am able to create a dataframe, but any operation I am running is leading to an error, with the exception of accessing the columns attribute itself:

In [1]: import pandas as pd

In [2]: pd.__version__
Out[2]: '1.1.3'

In [3]: df = pd.DataFrame([[1, 2], [3, 4]], columns=["pizza", ["a", "b"]])

In [4]: df.columns
Out[4]: Index(['pizza', ['a', 'b']], dtype='object')

In [5]: df
Out[5]: ---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
File pandas/_libs/hashtable_class_helper.pxi:1709, in pandas._libs.hashtable.PyObjectHashTable.map_locations()

TypeError: unhashable type: 'list'
Exception ignored in: 'pandas._libs.index.IndexEngine._call_map_locations'
Traceback (most recent call last):
  File "pandas/_libs/hashtable_class_helper.pxi", line 1709, in pandas._libs.hashtable.PyObjectHashTable.map_locations
TypeError: unhashable type: 'list'

if that's the case, len(set(...)) would raise the same error, and we don't need to necessarily/explicitly do it as well - at least for now. If eventually scikit-learn starts developing and find issues, we can change it or add a specific check for pandas 🤔

[dangotbanned]

I doubt the overhead is noticeable, a comment with a link to the pandas issue should be fine

It isn't necessarily the overhead per-call that I'm worried about, the issue is that this check is on most/all method calls - potentially multiple times.
I really feel like a stronger case needs to be made to justify this

[MarcoGorelli]

I am able to create a dataframe, but any operation I am running, is leading to an error

hmmm ok thanks, that is a sign that such a state is so broken that we shouldn't deal with it

[MarcoGorelli]

thanks both!

[dangotbanned]

thanks both!

Thanks @MarcoGorelli! 😍