Add support for flexible column lengths #5679

Posnet · 2024-04-22T15:43:20Z

Similar to what is supported in the csv crate, as well as the pandas, arrow-cpp and polars crates. A subset of CSV files treat missing columns at the end of rows as null (if the schema allows it). This commit adds support to optionally enable treating such missing columns as null. The default behavior is still to treat an incorrect number of columns as an error.

Which issue does this PR close?

Closes #5678

What changes are included in this PR?

Added boolean config fields to Format and Decoder to allow for specifying whether flexible columns are desired. And provide the ability to set when via builder methods.
The change also passes down the new flexible fields to the rust csv config.
The change also updates the decoder such that if flexible columns are enabled it will pad the offset buffers with empty offsets in order to allow for null values to be interpreted at parsing.
The change also adds unit tests for verify the new behavior.

Are there any user-facing changes?

Yes, it extends the API surface of the Format struct and ReaderBuilder struct. It also adds a set_flexible_lengths method to the RecordDecoder struct since adding the field to the new constructor would be a public api break. I believe the changes are only semantically minor.

Jefffrey · 2024-04-22T21:33:47Z

arrow-csv/src/reader/mod.rs

@@ -265,6 +266,14 @@ impl Format {
        self
    }

+    /// Whether to allow flexible lengths for records.


Suggested change

/// Whether to allow flexible lengths for records.

/// Whether to allow flexible number of columns for records.

/// If enabled, records with not enough delimited values compared to expected

/// number of columns will be padded with nulls for the missing columns.

Maybe something like this to make it clearer?

Yes, I'm not sure how verbose to be in the naming. I've always called them ragged rows, but a colleague said that was confusing as well.

Maybe being even more explicit as allow_trailing_empty_values_without_separarator would be better?

What do you think about incomplete_rows?

I've pushed a new version with allow_truncated_rows. It's like incomplete rows, but maybe a bit clearer that it is only the end of the row that can be incomplete/missing.

truncated_rows does sound clearer. Though, with_allow_truncated_rows() does seem a bit unwieldy as a name; do you think with_truncated_rows() might be sufficient?

arrow-csv/test/data/flexible_columns.csv

Jefffrey · 2024-04-23T09:20:02Z

arrow-csv/src/reader/mod.rs

@@ -265,6 +266,14 @@ impl Format {
        self
    }

+    /// Whether to allow flexible lengths for records.


What do you think about incomplete_rows?

Posnet · 2024-04-30T00:50:58Z

I've pushed a new version with clearer language, specifically with the option allow_truncated_rows since truncated can only mean the end of something is missing, and we are actually truncating a row (i.e. less columns) not truncating the column itself.

I've also added more tests to ensure that the default case returns the expected error,

and a test for non-nullable schemas to ensure they still return an error even if truncated rows are allowed.

Jefffrey

Looks pretty good, just a few minor notes. I don't think the CI failures are related to this PR, will need to check separately

Jefffrey · 2024-05-06T09:02:06Z

arrow-csv/test/data/truncated_rows.csv

+Name,Age,Occupation,DOB
+A1,34,Engineer,1985-07-16
+B2,29,Doctor
+,,


I'm curious if a completely empty line would introduce just a row of nulls?

Empty rows are ignored, which appears to be part of the default behavior of CSV core.
I've updated the tests for truncated rows to include ensuring this is the case (i.e. in case the default changes in future, someone will notice when the tests break).

Jefffrey · 2024-05-06T09:02:38Z

arrow-csv/src/reader/records.rs

+    /// Sets the decoder to allow rows with less than the expected number columns
+    pub fn set_allow_truncated_rows(&mut self, allow: bool) {
+        self.allow_truncated_rows = allow;
+    }


Maybe we can put this directly in the new()? I don't think RecordDecoder is a public API so should be good to change new() 🤔

Makes sense it isn't on Docs.rs, but it was marked pub so I wasn't sure.
I've made this change as a second commit in case it needs to be removed.

Jefffrey · 2024-05-06T09:06:07Z

arrow-csv/src/reader/records.rs

+                            if self.allow_truncated_rows && self.current_field < self.num_columns {
+                                // If the number of fields is less than expected, pad with nulls
+                                let fill_count = self.num_columns - self.current_field;
+                                let fill_value = self.offsets[self.offsets_len - 1];
+                                self.offsets[self.offsets_len..self.offsets_len + fill_count]
+                                    .fill(fill_value);
+                                self.offsets_len += fill_count;
+                            } else {
+                                return Err(ArrowError::CsvError(format!(
+                                    "incorrect number of fields for line {}, expected {} got {}",
+                                    self.line_number, self.num_columns, self.current_field
+                                )));
+                            }


Not too familiar with the RecordDecoder code but this looks good to me 👍

Jefffrey · 2024-05-06T09:07:46Z

arrow-csv/src/reader/mod.rs

@@ -265,6 +266,14 @@ impl Format {
        self
    }

+    /// Whether to allow flexible lengths for records.


truncated_rows does sound clearer. Though, with_allow_truncated_rows() does seem a bit unwieldy as a name; do you think with_truncated_rows() might be sufficient?

Similar to what is supported in the csv crate, as well as the pandas, arrow-cpp and polars crates. A subset of CSV files treat missing columns at the end of rows as null (if the schema allows it). This commit adds support to optionally enable treating such missing columns as null. The default behavior is still to treat an incorrect number of columns as an error.

Jefffrey · 2024-05-07T10:40:23Z

Thanks for this; I think this should be good to go once the clippy lint is fixed (can ignore docs and integration CI failures, are unrelated)

Instead of using a setter, truncated rows is passwed into the `new` method for RecordDecoder since it is not part of the public API.

Jefffrey · 2024-05-08T09:48:20Z

Thanks for this @Posnet 👍

github-actions bot added the arrow Changes to the arrow crate label Apr 22, 2024

Jefffrey reviewed Apr 22, 2024

View reviewed changes

Jefffrey reviewed Apr 23, 2024

View reviewed changes

Posnet force-pushed the Posnet/flexible-csv branch 4 times, most recently from 290b5f9 to 452f122 Compare April 30, 2024 00:46

Posnet force-pushed the Posnet/flexible-csv branch 2 times, most recently from d807a4e to ff6000c Compare May 5, 2024 07:29

Jefffrey approved these changes May 6, 2024

View reviewed changes

Posnet force-pushed the Posnet/flexible-csv branch from 6f57ab4 to 37b3550 Compare May 6, 2024 21:28

Posnet force-pushed the Posnet/flexible-csv branch 2 times, most recently from a3e5f21 to 4aacb68 Compare May 6, 2024 22:35

Add truncated rows to RecordDecoder::new

2aa0bc9

Instead of using a setter, truncated rows is passwed into the `new` method for RecordDecoder since it is not part of the public API.

Posnet force-pushed the Posnet/flexible-csv branch from 4aacb68 to 2aa0bc9 Compare May 7, 2024 12:16

Jefffrey merged commit 4045fb5 into apache:master May 8, 2024
20 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for flexible column lengths #5679

Add support for flexible column lengths #5679

Posnet commented Apr 22, 2024 •

edited

Jefffrey Apr 22, 2024

Posnet Apr 22, 2024

Jefffrey Apr 23, 2024

Posnet Apr 30, 2024

Jefffrey May 6, 2024 •

edited

Jefffrey Apr 23, 2024

Posnet commented Apr 30, 2024

Jefffrey left a comment

Jefffrey May 6, 2024

Posnet May 6, 2024

Jefffrey May 6, 2024

Posnet May 6, 2024

Jefffrey May 6, 2024

Jefffrey May 6, 2024 •

edited

Jefffrey commented May 7, 2024 •

edited

Jefffrey commented May 8, 2024

-    /// Whether to allow flexible lengths for records.
+    /// Whether to allow flexible number of columns for records.
+    /// If enabled, records with not enough delimited values compared to expected
+    /// number of columns will be padded with nulls for the missing columns.

Add support for flexible column lengths #5679

Add support for flexible column lengths #5679

Conversation

Posnet commented Apr 22, 2024 • edited

Which issue does this PR close?

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jefffrey May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Posnet commented Apr 30, 2024

Jefffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jefffrey May 6, 2024 • edited

Choose a reason for hiding this comment

Jefffrey commented May 7, 2024 • edited

Jefffrey commented May 8, 2024

Posnet commented Apr 22, 2024 •

edited

Jefffrey May 6, 2024 •

edited

Jefffrey May 6, 2024 •

edited

Jefffrey commented May 7, 2024 •

edited