Repository: modin-project/modin
Pull request: FIX-#3983: FIX-#4107: Materialize 'rowid' columns when selecting rows by position
<!--
Thank you for your contribution!
Please review the contributing docs: https://modin.readthedocs.io/en/latest/development/contributing.html
if you have questions about contributing.
-->

## What do these changes do?
Materialize 'rowid' columns when selecting rows by position. Otherwise, it defaults to range, that is not correct, when selecting particular rows.

<!-- Please give a short brief about these changes. -->

- [x] commit message follows format outlined [here](https://modin.readthedocs.io/en/latest/development/contributing.html#commit-message-formatting)
- [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 #3983
- [x] Resolves #4107
- [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 -->
- [x] added (Issue Number: PR title (PR Number)) and github username to release notes for next major release <!-- e.g. DOCS-#4077: Add release notes template to docs folder (#4078) -->


Problem statement:
``Series.iloc()`` and ``Series.loc()`` don`t work correctly at MODIN_STORAGE_FORMAT=omnisci
When I run:
```
import pandas
import modin.pandas as pd
from modin.pandas.test.utils import test_data_values, df_equals
import modin.config as cfg
cfg.StorageFormat.put('omnisci')

data = test_data_values[0]
modin_series = pd.Series(data[next(iter(data.keys()))])
pandas_series = pandas.Series(data[next(iter(data.keys()))])
df_equals(modin_series.iloc[1:], pandas_series.iloc[1:])
```
I have error:
```
AssertionError: Series.index are different

Series.index values are different (100.0 %)
[left]:  RangeIndex(start=0, stop=255, step=1)
[right]: RangeIndex(start=1, stop=256, step=1)
```
Also function ``loc`` have this problem.

``Series.__str__`` and ``Series.__repr__`` don`t work correctly at MODIN_STORAGE_FORMAT=omnisci
When I run:
```
import pandas 
import modin.pandas as pd
from modin.pandas.test.utils import test_data_values
import modin.config as cfg
cfg.StorageFormat.put('omnisci')

data = test_data_values[0]
modin_series = pd.Series(data[next(iter(data.keys()))])
pandas_series = pandas.Series(data[next(iter(data.keys()))])
assert str(modin_series) == str(pandas_series)
```
I have error:
```
AssertionError: assert '0           ...type: float64' == '0           ...type: float64'
  - 0            NaN
  ?  -
  + 0           NaN
  - 1      17.911928
  ?  -
  + 1     17.911928
  - 2      10.325881...

   ...Full output truncated (33 lines hidden), use '-vv' to show
```
Also function ``__repr__`` have this problem.

Hints from issue discussion:
@modin-project/modin-omnisci  could someone please check if this still happens?
@modin-project/modin-omnisci  could someone please check if this still happens?

Commit message: FIX-#3983: FIX-#4107: Materialize 'rowid' columns when selecting rows by position

 * Materialize the frame when masking by row positions.
 * Masking by labels is not supported.
 * Use the last column in column.py since, after the materialization,
   the first column, of the underlying arrow table, is the index.

Signed-off-by: Andrey Pavlenko <andrey.a.pavlenko@gmail.com>

Patch to review:
diff --git a/docs/release_notes/release_notes-0.16.0.rst b/docs/release_notes/release_notes-0.16.0.rst
index 332b339d6e9..8a600145d48 100644
--- a/docs/release_notes/release_notes-0.16.0.rst
+++ b/docs/release_notes/release_notes-0.16.0.rst
@@ -43,6 +43,7 @@ Key Features and Updates
   * FIX-#4848: Fix rebalancing partitions when NPartitions == 1 (#4874)
   * FIX-#4907: Implement `radd` for Series and DataFrame (#4908)
   * FIX-#4818, PERF-#4825: Fix where by using the new n-ary operator (#4820)    
+  * FIX-#3983: FIX-#4107: Materialize 'rowid' columns when selecting rows by position (#4834)
 * Performance enhancements
   * PERF-#4182: Add cell-wise execution for binary ops, fix bin ops for empty dataframes (#4391)
   * PERF-#4288: Improve perf of `groupby.mean` for narrow data (#4591)
diff --git a/modin/experimental/core/execution/native/implementations/omnisci_on_native/dataframe/dataframe.py b/modin/experimental/core/execution/native/implementations/omnisci_on_native/dataframe/dataframe.py
index 57b757945d2..a64be67c3cd 100644
--- a/modin/experimental/core/execution/native/implementations/omnisci_on_native/dataframe/dataframe.py
+++ b/modin/experimental/core/execution/native/implementations/omnisci_on_native/dataframe/dataframe.py
@@ -333,14 +333,18 @@ def take_2d_labels_or_positional(
                 force_execution_mode=self._force_execution_mode,
             )
 
-        if row_labels is not None or row_positions is not None:
+        if row_labels is not None:
+            raise NotImplementedError("row indices masking is not yet supported")
+
+        if row_positions is not None:
+            base = base._maybe_materialize_rowid()
             op = MaskNode(base, row_labels=row_labels, row_positions=row_positions)
             return self.__constructor__(
                 columns=base.columns,
                 dtypes=base._dtypes,
                 op=op,
-                index_cols=self._index_cols,
-                force_execution_mode=self._force_execution_mode,
+                index_cols=base._index_cols,
+                force_execution_mode=base._force_execution_mode,
             )
 
         return base
diff --git a/modin/experimental/core/execution/native/implementations/omnisci_on_native/interchange/dataframe_protocol/column.py b/modin/experimental/core/execution/native/implementations/omnisci_on_native/interchange/dataframe_protocol/column.py
index 9d6b0b8cd3f..9c98036f90f 100644
--- a/modin/experimental/core/execution/native/implementations/omnisci_on_native/interchange/dataframe_protocol/column.py
+++ b/modin/experimental/core/execution/native/implementations/omnisci_on_native/interchange/dataframe_protocol/column.py
@@ -75,7 +75,7 @@ def offset(self) -> int:
         # no longer depend on their parent tables. So materializing buffers
         # before returning the offset
         self._materialize_actual_buffers()
-        return self._pyarrow_table.column(0).chunks[0].offset
+        return self._pyarrow_table.column(-1).chunks[0].offset
 
     @property
     def dtype(self) -> Tuple[DTypeKind, int, str, str]:
@@ -188,7 +188,7 @@ def describe_categorical(self) -> CategoricalDescription:
         # Although we can retrieve codes from pandas dtype, they're unsynced with
         # the actual PyArrow data most of the time. So getting the mapping directly
         # from the materialized PyArrow table.
-        col = self._pyarrow_table.column(0)
+        col = self._pyarrow_table.column(-1)
         if len(col.chunks) > 1:
             if not self._col._allow_copy:
                 raise_copy_alert(
@@ -216,7 +216,7 @@ def describe_categorical(self) -> CategoricalDescription:
 
     @property
     def describe_null(self) -> Tuple[ColumnNullType, Any]:
-        null_buffer = self._pyarrow_table.column(0).chunks[0].buffers()[0]
+        null_buffer = self._pyarrow_table.column(-1).chunks[0].buffers()[0]
         if null_buffer is None:
             return (ColumnNullType.NON_NULLABLE, None)
         else:
@@ -224,7 +224,7 @@ def describe_null(self) -> Tuple[ColumnNullType, Any]:
 
     @property
     def null_count(self) -> int:
-        return self._pyarrow_table.column(0).null_count
+        return self._pyarrow_table.column(-1).null_count
 
     @property
     def metadata(self) -> Dict[str, Any]:
@@ -239,7 +239,7 @@ def _pandas_dtype(self) -> np.dtype:
         -------
         numpy.dtype
         """
-        return self._col._df.dtypes.iloc[0]
+        return self._col._df.dtypes.iloc[-1]
 
     @property
     def _arrow_dtype(self) -> pa.DataType:
@@ -250,7 +250,7 @@ def _arrow_dtype(self) -> pa.DataType:
         -------
         pyarrow.DataType
         """
-        return self._pyarrow_table.column(0).type
+        return self._pyarrow_table.column(-1).type
 
     @property
     def _pyarrow_table(self) -> pa.Table:
@@ -273,7 +273,8 @@ def get_chunks(self, n_chunks: Optional[int] = None) -> Iterable[ProtocolColumn]
     def get_buffers(self) -> Dict[str, Any]:
         self._materialize_actual_buffers()
         at = self._pyarrow_table
-        pyarrow_array = at.column(0).chunks[0]
+        # Get the last column since the first one could be the index
+        pyarrow_array = at.column(-1).chunks[0]
 
         result = dict()
         result["data"] = self._get_data_buffer(pyarrow_array)
@@ -481,10 +482,10 @@ def _propagate_dtype(self, dtype: Tuple[DTypeKind, int, str, str]):
 
         at = self._pyarrow_table
         schema_to_cast = at.schema
-        field = at.schema[0]
+        field = at.schema[-1]
 
         schema_to_cast = schema_to_cast.set(
-            0, pa.field(field.name, arrow_type, field.nullable)
+            len(schema_to_cast) - 1, pa.field(field.name, arrow_type, field.nullable)
         )
 
         # TODO: currently, each column chunk casts its buffers independently which results
diff --git a/modin/experimental/core/execution/native/implementations/omnisci_on_native/test/test_dataframe.py b/modin/experimental/core/execution/native/implementations/omnisci_on_native/test/test_dataframe.py
index ee3f2294988..d8d4f514b21 100644
--- a/modin/experimental/core/execution/native/implementations/omnisci_on_native/test/test_dataframe.py
+++ b/modin/experimental/core/execution/native/implementations/omnisci_on_native/test/test_dataframe.py
@@ -2169,5 +2169,51 @@ def test_set_index(self):
         df._query_compiler._modin_frame._set_index(pd.Index([1, 2, 3]))
 
 
+class TestLoc:
+    def test_loc(self):
+        data = [1, 2, 3, 4, 5, 6]
+        idx = ["a", "b", "c", "d", "e", "f"]
+        key = ["b", "c", "d", "e"]
+        mdf = pd.DataFrame(data, index=idx).loc[key]
+        pdf = pandas.DataFrame(data, index=idx).loc[key]
+        df_equals(mdf, pdf)
+
+    def test_iloc_bol(self):
+        data = [1, 2, 3, 4, 5, 6]
+        idx = ["a", "b", "c", "d", "e", "f"]
+        key = [False, True, True, True, True, False]
+        mdf = pd.DataFrame(data, index=idx).iloc[key]
+        pdf = pandas.DataFrame(data, index=idx).iloc[key]
+        df_equals(mdf, pdf)
+
+    def test_iloc_int(self):
+        data = range(11, 265)
+        key = list(range(0, 11)) + list(range(243, 254))
+        mdf = pd.DataFrame(data).iloc[key]
+        pdf = pandas.DataFrame(data).iloc[key]
+        df_equals(mdf, pdf)
+
+        mdf = pd.DataFrame(data).iloc[range(10, 100)]
+        pdf = pandas.DataFrame(data).iloc[range(10, 100)]
+        df_equals(mdf, pdf)
+
+        data = test_data_values[0]
+        mds = pd.Series(data[next(iter(data.keys()))]).iloc[1:]
+        pds = pandas.Series(data[next(iter(data.keys()))]).iloc[1:]
+        df_equals(mds, pds)
+
+
+class TestStr:
+    def test_str(self):
+        data = test_data_values[0]
+        mdf = pd.DataFrame(data[next(iter(data.keys()))])
+        pdf = pandas.DataFrame(data[next(iter(data.keys()))])
+        df_equals(mdf, pdf)
+
+        mds = pd.Series(data[next(iter(data.keys()))])
+        pds = pandas.Series(data[next(iter(data.keys()))])
+        assert str(mds) == str(pds)
+
+
 if __name__ == "__main__":
     pytest.main(["-v", __file__])


Code review comments:
diff_hunk: @@ -2169,5 +2169,51 @@ def test_set_index(self):
         df._query_compiler._modin_frame._set_index(pd.Index([1, 2, 3]))
 
 
+class TestLoc:
+    def test_loc(self):
+        data = [1, 2, 3, 4, 5, 6]
+        idx = ["a", "b", "c", "d", "e", "f"]
+        key = ["b", "c", "d", "e"]
+        mdf = pd.DataFrame(data, index=idx).loc[key]
+        pdf = pandas.DataFrame(data, index=idx).loc[key]
+        df_equals(mdf, pdf)
+
+    def test_iloc_bol(self):
path: modin/experimental/core/execution/native/implementations/omnisci_on_native/test/test_dataframe.py
text: @user1:
```suggestion
    def test_iloc_bool(self):
```