chore!: remove pyarrow-based file reader #3587
Conversation
BREAKING CHANGE: reading using PyArrow, along with the PythonStorageConfig, are removed
CodSpeed Performance ReportMerging #3587 will degrade performances by 36.08%Comparing Summary
Benchmarks breakdown
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3587 +/- ##
==========================================
- Coverage 77.86% 77.65% -0.22%
==========================================
Files 719 717 -2
Lines 88459 88204 -255
==========================================
- Hits 68877 68492 -385
- Misses 19582 19712 +130
|
| @@ -17,7 +17,8 @@ impl MonotonicallyIncreasingId { | |||
| let column_name = column_name.unwrap_or("id"); | |||
|
|
|||
| let mut schema_with_id_index_map = input.schema().fields.clone(); | |||
| schema_with_id_index_map.insert( | |||
| schema_with_id_index_map.shift_insert( | |||
| 0, | |||
There was a problem hiding this comment.
Why? Seems like an orthogonal change to the rest of the PR
There was a problem hiding this comment.
Ok this is a long explanation but it's necessary.
One discrepancy between the native and python readers I found is that if you pass in a schema that reorders the columns, the native reader does not actually respect that. The reason turned out to be because an unloaded micropartition only creates a new scan task if the schemas differ (see here). However, the impl of Eq for Schema relies on Eq for indexmap::IndexMap, which does not actually check for ordering. Thus the schemas are erronenously presumed to be equal.
So, I made the equality check for Schema stricter, however that caused an issue with monotonically increasing ID, since the schema built in the logical plan (which has the ID at the end) was different from the schema of the actual computed tables (which have the ID as the first column). That's why I need to shift_insert instead of insert here.
There was a problem hiding this comment.
Whaaaat 😮
Ok can we lift this to the PR description? Very surprised that this was surfaced only now though, seems like a bug that should have come up sooner.
| read_options=json_read_options, | ||
| io_config=config.io_config, | ||
| ) | ||
| return _cast_table_to_schema(tbl, read_options=read_options, schema=schema) | ||
|
|
||
|
|
||
| def read_parquet( |
There was a problem hiding this comment.
What do we need table_io.read_parquet/csv/json for? Is it possible to get rid of them even?
There was a problem hiding this comment.
You're right, I believe we eliminate the Python code path entirely. I think I'll leave that for a future PR and add a TODO here.
There was a problem hiding this comment.
In fact we could probably remove table_io.py entirely if we port our non-swordfish code to use daft.io.writer to do writing.
|
BTW we can maybe just deprecate the APIs first and deal with this later In the interest of letting us cut a release first, I might push up an API-only change first since it seems this PR might not yet be ready to go. |
BREAKING CHANGE: removed the following: reading using PyArrow,
PythonStorageConfigtype andStorageConfig.pythonfunction,daft.table.schema_inference, refactoredStorageConfigandNativeStorageConfiginto one typeAlso includes two small fixes:
Schemastruct equality now also depends on the ordering of the fieldstodo: