[Data] Simplify ArrowBlock and PandasBlock #58883
Conversation
Signed-off-by: Srinath Krishnamachari <srinath.krishnamachari@anyscale.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies ArrowBlock and PandasBlock by removing the TableRow base class and moving its functionality directly into ArrowRow and PandasRow, or into helper functions. This is a good refactoring that reduces complexity and improves code clarity. The changes are consistent across the modified files. I've identified a couple of minor issues with incorrect type hints that should be addressed to enhance maintainability.
| @staticmethod | ||
| def _build_tensor_row( | ||
| row: ArrowRow, col_name: str = TENSOR_COLUMN_NAME | ||
| row: ArrowRow, row_idx: int, col_name: str = TENSOR_COLUMN_NAME |
There was a problem hiding this comment.
The type hint for the row parameter is ArrowRow, but it appears to be incorrect. The only call site in this file, ArrowRow.__getitem__, passes self._row, which is a pyarrow.Table. If row were an ArrowRow instance, the expression row[col_name] within this method would trigger a recursive call to ArrowRow.__getitem__. To improve type safety and clarity, please update the type hint to row: "pyarrow.Table".
| row: ArrowRow, row_idx: int, col_name: str = TENSOR_COLUMN_NAME | |
| row: "pyarrow.Table", row_idx: int, col_name: str = TENSOR_COLUMN_NAME |
|
|
||
| @staticmethod | ||
| def _build_tensor_row(row: PandasRow) -> np.ndarray: | ||
| def _build_tensor_row(row: PandasRow, row_idx: int) -> np.ndarray: |
There was a problem hiding this comment.
The type hint for the row parameter is PandasRow, which seems incorrect. If row were a PandasRow instance, row[TENSOR_COLUMN_NAME] would call PandasRow.__getitem__, which returns a tuple. Calling .iloc[row_idx] on a tuple would raise an error. The row parameter is likely expected to be a pandas.DataFrame. Please update the type hint to row: "pandas.DataFrame" for correctness and clarity.
| def _build_tensor_row(row: PandasRow, row_idx: int) -> np.ndarray: | |
| def _build_tensor_row(row: "pandas.DataFrame", row_idx: int) -> np.ndarray: |
Description
[Data] Simplify ArrowBlock and PandasBlock
Simplify inheritance hierarchy for
ArrowBlockandPandasBlockby removingTableRowto improve code maintainability.Related issues
Additional information