Repository: modin-project/modin
Pull request: FIX-#3925: Fixed AssertionError on columns and index drop
## What do these changes do?

1. Index drop is not supported by the HDK engine. In this case - fall back to pandas.
2. If all columns are dropped and the index is trivial, we are not able to restore it, since we don't know the number of rows. In this case, we calculate the index from the current frame.

**Fixes #3916**

<!-- Please give a short brief about these changes. -->

- [x] first commit message and PR title follow format outlined [here](https://modin.readthedocs.io/en/latest/development/contributing.html#commit-message-formatting)
  > **_NOTE:_**  If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.
- [x] passes `flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py`
- [x] passes `black --check modin/ asv_bench/benchmarks scripts/doc_checker.py`
- [x] signed commit with `git commit -s` <!-- you can amend your commit with a signature via `git commit -amend -s` -->
- [x] Resolves #3925? <!-- issue must be created for each patch -->
- [x] tests added and passing
- [ ] module layout described at `docs/development/architecture.rst` is up-to-date <!-- if you have added, renamed or removed files or directories please update the documentation accordingly -->


Problem statement:
``series.___delitem__`` doesn`t work correctly at MODIN_STORAGE_FORMAT=omnisci
When I run:
```
import modin.pandas as pd
from modin.pandas.test.utils import test_data
import modin.config as cfg
cfg.StorageFormat.put('omnisci')

data = test_data["float_nan_data"]
modin_series = pd.Series(data[next(iter(data.keys()))])
del modin_series[modin_series.index[0]]
```
I have error:
```
AssertionError: Only column drop is supported
```

``dataframe.drop()`` doesn`t work correctly at MODIN_STORAGE_FORMAT=omnisci
When I run:
```
import pandas
import modin.pandas as pd
from modin.pandas.test.utils import df_equals
import modin.config as cfg
cfg.StorageFormat.put('omnisci')

frame_data = {"A": [1, 2, 3, 4], "B": [0, 1, 2, 3]}
simple = pandas.DataFrame(frame_data)
modin_simple = pd.DataFrame(frame_data)
df_equals(modin_simple.drop(["A", "B"], axis="columns"), simple[[]])
```
I have error:
```
AssertionError: DataFrame are different

DataFrame shape mismatch
[left]:  (0, 0)
[right]: (4, 0)
```
Also ``drop`` have next problem. When I run:
```
import pandas
import modin.pandas as pd
import modin.config as cfg
cfg.StorageFormat.put('omnisci')

frame_data = [[1, 2, 3], [3, 4, 5], [5, 6, 7]]
modin_df = pd.DataFrame(frame_data, index=["a", "b", "c"], columns=["d", "e", "f"])
modin_df1 = modin_df.drop("a")
```
I have error:
```
AssertionError: Only column drop is supported
```

Hints from issue discussion:
This issue should be fixed by #5103 and #5156.
Please link this issue to the PR which is going to resolve it.
> Please link this issue to the PR which is going to resolve it.

Added links to both PRs. Both fixes are required to resolve this issue.
@modin-project/modin-omnisci  could someone please check if this still happens?

Commit message: FIX-#3925: Fixed AssertionError on columns and index drop

1. Index drop is not supported by the HDK engine. In this case - fall
   back to pandas.
2. If all columns are dropped and the index is trivial, we are
   not able to restore it, since we don't know the number of rows.
   In this case, we calculate the index from the current frame.

Signed-off-by: Andrey Pavlenko <andrey.a.pavlenko@gmail.com>
Co-authored-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

Patch to review:
diff --git a/modin/experimental/core/execution/native/implementations/hdk_on_native/dataframe/dataframe.py b/modin/experimental/core/execution/native/implementations/hdk_on_native/dataframe/dataframe.py
index 81bb4a304af..c27945060cd 100644
--- a/modin/experimental/core/execution/native/implementations/hdk_on_native/dataframe/dataframe.py
+++ b/modin/experimental/core/execution/native/implementations/hdk_on_native/dataframe/dataframe.py
@@ -2506,15 +2506,21 @@ def to_pandas(self):
         # index columns.
         if len(df.columns) != len(self.columns):
             assert self._index_cols
-            df.set_index([f"F_{col}" for col in self._index_cols], inplace=True)
-            df.index.rename(self._index_names(self._index_cols), inplace=True)
-            assert len(df.columns) == len(self.columns)
+            idx_col_names = [f"F_{col}" for col in self._index_cols]
+            if self._index_cache is not None:
+                df.drop(columns=idx_col_names, inplace=True)
+                df.index = self._index_cache.copy()
+            else:
+                df.set_index(idx_col_names, inplace=True)
+                df.index.rename(self._index_names(self._index_cols), inplace=True)
         else:
             assert self._index_cols is None
             assert df.index.name is None, f"index name '{df.index.name}' is not None"
+            if self._index_cache is not None:
+                df.index = self._index_cache.copy()
 
         # Restore original column labels encoded in HDK to meet its
-        # restirctions on column names.
+        # restrictions on column names.
         df.columns = self.columns
 
         return df
diff --git a/modin/experimental/core/execution/native/implementations/hdk_on_native/test/test_dataframe.py b/modin/experimental/core/execution/native/implementations/hdk_on_native/test/test_dataframe.py
index 9f641b6801d..2ba6026696c 100644
--- a/modin/experimental/core/execution/native/implementations/hdk_on_native/test/test_dataframe.py
+++ b/modin/experimental/core/execution/native/implementations/hdk_on_native/test/test_dataframe.py
@@ -391,10 +391,20 @@ def projection(df, cols, **kwargs):
         run_and_compare(projection, data=self.data, cols=cols)
 
     def test_drop(self):
+        def drop(df, column_names, **kwargs):
+            return df.drop(columns=column_names)
+
+        run_and_compare(drop, data=self.data, column_names="a")
+        run_and_compare(drop, data=self.data, column_names=self.data.keys())
+
+    def test_drop_index(self):
         def drop(df, **kwargs):
-            return df.drop(columns="a")
+            return df.drop(df.index[0])
 
-        run_and_compare(drop, data=self.data)
+        idx = list(map(str, self.data["a"]))
+        run_and_compare(
+            drop, data=self.data, constructor_kwargs={"index": idx}, force_lazy=False
+        )
 
     def test_iloc(self):
         def mask(df, **kwargs):
diff --git a/modin/experimental/core/storage_formats/hdk/query_compiler.py b/modin/experimental/core/storage_formats/hdk/query_compiler.py
index 77864307352..9198c82275a 100644
--- a/modin/experimental/core/storage_formats/hdk/query_compiler.py
+++ b/modin/experimental/core/storage_formats/hdk/query_compiler.py
@@ -557,18 +557,27 @@ def concat(self, axis, other, **kwargs):
 
     def drop(self, index=None, columns=None, errors: str = "raise"):
         if index is not None:
+            # Only column drop is supported by the HDK engine
             raise NotImplementedError("Row drop")
         if errors != "raise":
             raise NotImplementedError(
                 "This lazy query compiler will always "
                 + "raise an error on invalid columns."
             )
-        return self.__constructor__(
-            self._modin_frame.take_2d_labels_or_positional(
-                row_labels=index, col_labels=self.columns.drop(columns)
-            )
+
+        columns = self.columns.drop(columns)
+        new_frame = self._modin_frame.take_2d_labels_or_positional(
+            row_labels=index, col_labels=columns
         )
 
+        # If all columns are dropped and the index is trivial, we are
+        # not able to restore it, since we don't know the number of rows.
+        # In this case, we copy the index from the current frame.
+        if len(columns) == 0 and new_frame._index_cols is None:
+            new_frame._index_cache = self._modin_frame.index.copy()
+
+        return self.__constructor__(new_frame)
+
     def dropna(self, axis=0, how=no_default, thresh=no_default, subset=None):
         if thresh is not no_default or axis != 0:
             raise NotImplementedError(


Code review comments:
diff_hunk: @@ -557,18 +557,27 @@ def concat(self, axis, other, **kwargs):
 
     def drop(self, index=None, columns=None, errors: str = "raise"):
         if index is not None:
+            # Only column drop is supported by the HDK engine
             raise NotImplementedError("Row drop")
         if errors != "raise":
             raise NotImplementedError(
                 "This lazy query compiler will always "
                 + "raise an error on invalid columns."
             )
-        return self.__constructor__(
-            self._modin_frame.take_2d_labels_or_positional(
-                row_labels=index, col_labels=self.columns.drop(columns)
-            )
+
+        columns = self.columns.drop(columns)
+        new_frame = self._modin_frame.take_2d_labels_or_positional(
+            row_labels=index, col_labels=columns
         )
 
+        # If all columns are dropped and the index is trivial, we are
+        # not able to restore it, since we don't know the number of rows.
+        # In this case, we copy the index from the current frame.
+        if len(columns) == 0 and new_frame._index_cols is None:
+            new_frame._index_cache = self._modin_frame.index.copy()
path: modin/experimental/core/storage_formats/hdk/query_compiler.py
text: @user1:
let's add this assertion just to be sure we're safe here
```suggestion
        if len(columns) == 0 and new_frame._index_cols is None:
            assert index is None, "Can't copy old indexes as there was a row drop"
            new_frame._index_cache = self._modin_frame.index.copy()
```

@user2:
Let's use `ValueError` exception, it's more reliable.

@author:
The index as always None here. There is a check on the line 559.

@user1:
Yes, I know that. The `assert` is just another safety measure for the case we remove the check on 559 line someday.