AndreaBozzo commented on PR #10903: URL: https://github.com/apache/arrow-rs/pull/10903#issuecomment-5506933774
Answering as the consumer, since dataprof is the only thing driving this today. **What this PR buys me.** [This schema-widening hack](https://github.com/AndreaBozzo/dataprof/blob/a6a7519cf26009b74ddae5c2429db4ffe38ab76c/crates/dataprof-parquet/src/arrow_profiler.rs#L207-L218) goes away: the Arrow path widens the schema to the widest record in the file and projects the surplus columns off, purely because a long row aborts the scan. `ExtraFields::Ignore` replaces it, and the `with_truncated_rows(true)` + `Ignore` pairing in `test_truncated_rows_with_extra_fields` is exactly [my configuration](https://github.com/AndreaBozzo/dataprof/blob/a6a7519cf26009b74ddae5c2429db4ffe38ab76c/crates/dataprof-parquet/src/arrow_profiler.rs#L226-L233), both driven from one "tolerate ragged rows" flag. On your `||` comment in that test: the second branch is dead either way. `Expected 3 records` is the `csv` crate's message, reachable only through `infer_schema` (see `test_record_length_mismatch`), while a `ReaderBuilder::build` path with an explicit schema only ever produces `RecordDecoder`'s `incorrect number of fields for line N, expected X got Y`. So the `incorrect number of fields` side is the one to assert exactly. **What it doesn't buy me,** and my only remaining ask: a row counter for the ignore path, mirroring `truncated_row_count()`. dataprof reports a ragged-row count and pays [a second full read of the file](https://github.com/AndreaBozzo/dataprof/blob/a6a7519cf26009b74ddae5c2429db4ffe38ab76c/crates/dataprof-parquet/src/arrow_profiler.rs#L101-L112) for it, about 3% of wall time on a 218 MB / 2M-row profile. `truncated_row_count()` covers the short half; the long half retires that pass entirely. Purely additive, no deprecation. Separate PR, happy to open it. **On the shape question** I have no real stake. The bool costs me one call site either way, and it reaches DataFusion's `CsvOptions` and datafusion-python's documented options, so that churn isn't mine to spend. The one thing I would argue against is folding the two into a single option, and the inference issue is the argument: `csv::ReaderBuilder::flexible` is already one flag covering both directions, which is exactly how a long-row setting ended up changing short-row behaviour. Fields over columns, for the same reason csv-core and the existing message use it. That issue came from my suggestion, so: ```rust // infer_schema loop, after read_record if !self.truncated_rows && record.len() < header_length { let line = record.position().map(|p| p.line()).unwrap_or_default(); return Err(ArrowError::CsvError(format!( "incorrect number of fields for line {line}, expected {header_length} got {}", record.len() ))); } ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
