Skip to content

GH-50925: [C++] Allow CSV reader to pad rows with missing trailing fields - #50926

Open
HuaHuaY wants to merge 5 commits into
apache:mainfrom
HuaHuaY:strengthen_csv
Open

GH-50925: [C++] Allow CSV reader to pad rows with missing trailing fields#50926
HuaHuaY wants to merge 5 commits into
apache:mainfrom
HuaHuaY:strengthen_csv

Conversation

@HuaHuaY

@HuaHuaY HuaHuaY commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

Some CSV files omit trailing optional fields. The C++ CSV reader currently rejects these short rows, requiring callers to preprocess or discard them.

What changes are included in this PR?

Add an option pad_short_rows in CSV struct ParseOptions that pads missing trailing fields with nulls.

Are these changes tested?

Yes.

Are there any user-facing changes?

Add an option pad_short_rows in CSV struct ParseOptions.

@HuaHuaY

HuaHuaY commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

I'll temporarily switch to a draft and reorganize my PR.

@HuaHuaY
HuaHuaY marked this pull request as draft August 24, 2026 07:31
@HuaHuaY
HuaHuaY marked this pull request as ready for review August 25, 2026 09:15
Copilot AI lite review requested due to automatic review settings August 25, 2026 09:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new C++ CSV ParseOptions::pad_short_rows option to accept rows with fewer trailing fields than the expected column count, padding the missing trailing fields as nulls (instead of rejecting the row). This aligns the CSV reader behavior with common “optional trailing column” CSV inputs while keeping the default strict behavior unchanged.

Changes:

  • Introduce ParseOptions::pad_short_rows (default false) to enable padding of short rows with nulls.
  • Extend the CSV parser/visitor path to propagate “missing (padded)” cells to converters so they can reliably append nulls.
  • Add parser- and reader-level tests covering both string and typed/dictionary conversions.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
cpp/src/arrow/dataset/file_csv.cc Includes pad_short_rows in CsvFileFormat::Equals so formats with different padding behavior are not considered equal.
cpp/src/arrow/csv/options.h Adds the public ParseOptions::pad_short_rows option and documents its intent.
cpp/src/arrow/csv/parser.cc Implements padding behavior for short rows by appending empty field descriptors and recording missing ranges.
cpp/src/arrow/csv/parser.h Threads a missing flag through VisitColumn/VisitLastRow (backward-compatible visitor signature support).
cpp/src/arrow/csv/converter.cc Ensures padded “missing” values convert to nulls across supported converter paths (primitive + dictionary).
cpp/src/arrow/csv/parser_test.cc Adds unit coverage verifying padding and the missing signal in visitor callbacks.
cpp/src/arrow/csv/reader_test.cc Adds integration coverage for padded short rows for string columns and typed/dictionary columns.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cpp/src/arrow/csv/converter.cc
Comment thread cpp/src/arrow/csv/options.h
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 25, 2026
@HuaHuaY

HuaHuaY commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

@pitrou Please take a look.

Comment thread cpp/src/arrow/csv/parser.h Outdated
Copilot AI review requested due to automatic review settings August 25, 2026 13:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cpp/src/arrow/csv/parser.h:240

  • BlockParser::VisitColumn / VisitLastRow are public template APIs in a public header; changing the visitor signature to require an extra bool missing argument is a source-breaking change for downstream callers. Consider keeping backward compatibility by accepting both 3-arg and 4-arg visitors (e.g., use std::is_invocable_r_v<Status, Visitor, const uint8_t*, uint32_t, bool, bool> / if constexpr to dispatch), and document precisely what missing means (padded field due to pad_short_rows, not an empty field).
  /// \brief Visit parsed values in a column
  ///
  /// The signature of the visitor is
  /// Status(const uint8_t* data, uint32_t size, bool quoted, bool missing)
  template <typename Visitor>
  Status VisitColumn(int32_t col_index, Visitor&& visit) const {
    return parsed_batch().VisitColumn(col_index, first_row_num(),
                                      std::forward<Visitor>(visit));
  }

  template <typename Visitor>
  Status VisitLastRow(Visitor&& visit) const {
    return parsed_batch().VisitLastRow(std::forward<Visitor>(visit));
  }

Comment thread cpp/src/arrow/csv/reader.cc
Comment thread cpp/src/arrow/csv/parser_test.cc
Comment thread cpp/src/arrow/csv/parser_test.cc
Copilot AI review requested due to automatic review settings August 26, 2026 12:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

cpp/src/arrow/dataset/file_csv.cc:196

  • GetOrderedColumnNames() ignores the new missing flag from VisitLastRow(). If pad_short_rows is enabled and earlier parsed rows have more columns than the header row (e.g. due to skip_rows), this will currently append empty strings as column names for missing header fields, potentially creating empty/duplicate column names and confusing downstream errors. Consider failing fast when missing is true (or generating names explicitly).
      [&](const uint8_t* data, uint32_t size, bool quoted, bool missing) -> Status {
        std::string_view view{reinterpret_cast<const char*>(data), size};
        column_names.emplace_back(view);
        return Status::OK();
      }));

Comment thread cpp/src/arrow/csv/parser.h

@wgtmac wgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copilot AI review requested due to automatic review settings August 31, 2026 07:23
@HuaHuaY

HuaHuaY commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Rebase and add one more commit. Not allow to pad header row when using dataset reader.

In the situation, the number of columns is not determined by the number of headers in the file, which seems strange. However, changing this would result in behavior inconsistent with the existing code (even though the inconsistency would only arise with files that previously caused errors) and need more modifications. Therefore, I opted for the simple solution of temporarily disabling this newly added flag.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@pitrou

pitrou commented Aug 31, 2026

Copy link
Copy Markdown
Member

Therefore, I opted for the simple solution of temporarily disabling this newly added flag.

Wouldn't it be better to return Status::NotImplemented to signal that the option is not supported?

@HuaHuaY

HuaHuaY commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Therefore, I opted for the simple solution of temporarily disabling this newly added flag.

Wouldn't it be better to return Status::NotImplemented to signal that the option is not supported?

I intend for the newly added flag in this situation to still be usable for filling in missing columns in the data; it simply shouldn't fill in the header row of the CSV.

In the CSV reader, when the user does not explicitly provide column names, the number of columns is determined by the first row after skipping skip_rows (meaning that if the file contains a header row, the count reflects the number of column names).

However, in the dataset reader, skip_rows is not processed upfront, causing the column count to be determined before the file's header row is parsed (I think the practice of determining the column count based on skipped rows is strange and likely indicative of an imperfect earlier implementation).

@pitrou

pitrou commented Aug 31, 2026

Copy link
Copy Markdown
Member

I'm not sure I understand: if I enable pad_short_rows with the dataset reader, is the flag ignored or not?

@HuaHuaY

HuaHuaY commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

I'm not sure I understand: if I enable pad_short_rows with the dataset reader, is the flag ignored or not?

In this PR, it is ignored when parsing column names from the file, but remains effective when parsing the data.

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one nit

Comment thread cpp/src/arrow/dataset/file_csv.cc
Copilot AI review requested due to automatic review settings August 31, 2026 09:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants