[fix](be) Align CSV V2 enclosed field parsing#65501
Open
Gabriel39 wants to merge 4 commits into
Open
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FileScanner V2 used an independent CSV field-splitting state machine that diverged from the line reader for bare quotes, escapes outside enclosed fields, and configurations where escape equals enclose. It also removed double quotes before nullable string conversion, causing quoted null markers to be treated as NULL. Reuse the line reader's separator positions, preserve quoted-string provenance through null matching, and adjust separator positions when a UTF-8 BOM is removed.
### Release note
Fix CSV FileScanner V2 parsing for enclosed fields and quoted null literals.
### Check List (For Author)
- Test: Unit Test and Regression test
- CsvV2ReaderTest: 30 tests passed
- test_local_tvf_csv_enclose_consistency: passed
- Behavior changed: Yes. CSV V2 now matches the established enclosed-field parser and preserves quoted null literals as strings.
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review completed for PR 65501.
I found one correctness issue and requested changes inline.
Checkpoint conclusions:
- Goal/test: the PR improves CSV v2 enclosed-field parsing and adds focused BE/regression coverage, but the BOM-prefixed quoted first-field case is still broken.
- Scope: the code changes are focused on delimited CSV v2 parsing, BOM offset adjustment, quoted-null handling, and coverage.
- Concurrency/lifecycle: no shared concurrent state or non-obvious lifecycle issue found; reader contexts are per reader and refreshed per logical row.
- Config/compatibility/persistence: no new config, storage format, protocol, or persistence change found; old CSV behavior was compared where relevant.
- Parallel paths: v2 reader, old CSV reader comparison, unit tests, and local TVF regression path were reviewed. The actionable issue is specific to v2 consuming enclosed separator positions computed before BOM stripping.
- Testing: new tests cover ordinary enclosed separators, matching escape/enclose, NORMAL-state bare quotes/escapes, plain BOM, and quoted nulls, but should add BOM plus quoted first field with comma/newline.
- Performance/observability: no separate performance or observability concern found.
- User focus: no additional user-provided focus was supplied.
- Validation: static review only, per the review prompt; no builds or tests were run.
Subagent process: round 1 normal/risk subagents completed, candidate M1 was accepted, and round 2 convergence subagents returned NO_NEW_VALUABLE_FINDINGS against the final comment set.
### What problem does this PR solve? Issue Number: None Related PR: apache#65501 Problem Summary: FileScannerV2 removed a UTF-8 BOM only after the enclosed CSV line reader had already selected the first field state. A headerless record such as BOM plus `"a,b",2` was therefore treated as unquoted, and a newline inside that first quoted field could terminate the logical record early. Let the enclosed state machine structurally skip a first-record BOM before choosing its state while leaving byte removal and separator-offset adjustment to the V2 CSV reader. The option is enabled only for V2 reads starting at file offset zero, so legacy readers and non-first splits retain their existing behavior. ### Release note Fix FileScannerV2 parsing of BOM-prefixed CSV records whose first enclosed field contains separators or newlines. ### Check List (For Author) - Test: Unit Test - `CsvV2ReaderTest.*` (32 tests) - Behavior changed: Yes, V2 now recognizes an enclosed first field immediately following a UTF-8 BOM - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found three blocking issues in this PR.
Checkpoint conclusions:
- Goal fit: the PR improves CSV v2 enclosed-field consistency and quoted-null handling, but the new separator-position reuse still has correctness gaps in edge cases.
- Scope/focus: the code changes are mostly focused on CSV v2 parsing and its tests.
- Concurrency/lifecycle: no shared-thread lifecycle issue found; the relevant lifecycle issue is parser state preserved across one logical line reader refill, covered inline.
- Compatibility: two parser edge cases can now abort or split incorrectly; one new regression suite can fail in single-BE remote/isolated environments before reaching the parser behavior.
- Tests: added BE/regression coverage is useful, but it does not cover separator/delimiter overlap, cross-refill multi-character separators, or the single-BE fixture-copy path.
- Observability/config/persistence/transactions: no new config, persistence, transaction, FE-BE protocol, or observability requirement found for this change.
User focus: no additional user-provided review focus was supplied.
Validation note: this was a static review only, per the review-runner instructions; I did not run builds or tests.
### What problem does this PR solve? Issue Number: None Related PR: apache#65501 Problem Summary: CSV V2 could record column separators that overlap line delimiter bytes or re-record an accepted multi-character separator after an output-buffer refill, leading to invalid split offsets and BE checks. The local TVF regression suite also skipped fixture copies for single-BE environments. Limit separator searches to the returned logical line, clamp refill backtracking after the last accepted separator, copy fixtures for every backend, and add focused unit coverage. ### Release note Prevent CSV V2 failures when column separators overlap line delimiters or cross line-reader refill boundaries. ### Check List (For Author) - Test: Unit Test - CsvV2ReaderTest: 34 tests passed - Behavior changed: Yes. CSV V2 no longer records separator offsets outside the returned logical line or duplicates accepted separators across refills. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR #65501 (head 0be70b5):
I found no new distinct inline findings beyond the existing review threads already present on this PR.
Critical checkpoint conclusions:
- Goal/test coverage: The patch targets CSV v2 enclosed-field parsing by reusing the line-reader state machine for separator positions, BOM adjustment, refill-boundary handling, and quoted null behavior. Added BE unit coverage and a local TVF regression cover the main offset, BOM, delimiter-overlap, refill, matching escape/enclose, and quoted-null cases.
- Scope: The change is focused on the CSV/text line-reader and v2 delimited reader path, with no unrelated refactor found.
- Concurrency/lifecycle: No shared concurrent state or locking path is introduced. Parser state is per reader and refreshed per logical record; skipped/header rows do not reuse separator positions.
- Compatibility/parallel paths: Old CSV still constructs
EncloseCsvLineReaderCtxwith default no v2 BOM-skip behavior; the shared backtracking/search-bound changes were checked against old CSV splitting. No FE-BE thrift/config/storage-format/persistence changes were introduced. - Error handling/data correctness: Offset assertions in
_split_line()remain invariant checks; current head bounds separator search before delimiter bytes and adjusts BOM-shifted positions before splitting. - Tests: BE tests and the new regression are present and deterministic. The regression copies fixtures to each backend, pins
backend_id, enablesenable_file_scanner_v2, and uses ordered result checks. - Performance/observability: No new logging/metrics needed for this localized parser fix; added separator-position copying is limited to enclosed CSV splitting and no obvious hot-path regression stood out.
User focus: no additional review focus was provided.
Validation: Static review only, as requested by the review prompt. The local checkout is shallow/grafted and the base commit is not present, so an exact git diff --check range could not run; the bundled authoritative patch reverse-applied cleanly with whitespace warnings enabled.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#65501 Problem Summary: Hive text field splitting only inspected the byte immediately before a delimiter, so a delimiter after an even-length escape run was incorrectly treated as escaped in both V1 and V2. Hive OpenCSV planning also enabled double-quote trimming for custom quote characters, removing literal double quotes from fields. Count the complete escape run before delimiters, share the parity rule across both reader paths, and only enable double-quote trimming when the configured OpenCSV quote character is a double quote. ### Release note Match Hive LazySimpleSerDe escape parity and preserve literal double quotes with custom OpenCSV quote characters. ### Check List (For Author) - Test: Unit Test - HiveTextFieldSplitterTest, TextV2ReaderTest, and custom-enclose CsvV2ReaderTest: 32 tests passed - FE checkstyle passed; HiveScanNodeTest compilation was blocked by unrelated missing cloud storage SDK dependencies in the remote validation worktree - Behavior changed: Yes. Hive text delimiters after even escape runs now split fields, and custom OpenCSV quote characters no longer trim literal double quotes. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29513 ms |
Contributor
TPC-DS: Total hot run time: 180709 ms |
Contributor
ClickBench: Total hot run time: 24.98 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review completed for PR 65501 at head 0cd87392d5d5c0a390f8f6cdcdb69bb106cb240a.
I found no additional distinct issues to report in this round. I treated the existing inline review threads as already-known context and did not duplicate them. The current head appears to address the previously raised hazards around enclosed CSV separator positions, BOM-before-quote handling, output-buffer refill backtracking, and local TVF fixture staging.
Critical checkpoint conclusions:
- Goal and tests: the change aligns CSV FileScanner V2 enclosed-field splitting with the line-reader state machine, preserves quoted null literals, fixes Hive text even/odd escape handling, and narrows OpenCSV double-quote trimming to the double-quote enclosure case. The PR includes focused BE unit tests, an FE unit test, and a regression suite/output for the main behavior.
- Scope: the implementation is focused on CSV/text parsing, FE OpenCSV scan attributes, and directly related tests.
- Concurrency and lifecycle: no new concurrent shared state was introduced. The parser state is per reader; separator positions are cleared on each logical-line refresh and consumed before the next refresh.
- Compatibility and parallel paths: legacy CSV and Hive text paths were checked for parity. FE/BE thrift behavior remains compatible for default OpenCSV double quotes and custom quote characters.
- Error handling and data correctness: no ignored status or silent continuation issue was found in the changed control flow. The remaining separator-position assertions are supported by the current line-reader invariants.
- Test quality: added tests cover bare quotes/escapes, matching escape/enclose, custom enclosure, delimiter overlap, refill, BOM before quoted fields, quoted null literals, and Hive double escapes. I did not run builds or tests because this runner was instructed to perform review only.
User focus: no additional user-provided review focus was present.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FileScanner V2 used an independent CSV field-splitting state machine that diverged from the line reader for bare quotes, escapes outside enclosed fields, and configurations where escape equals enclose. It also removed double quotes before nullable string conversion, causing quoted null markers to be treated as NULL. This change reuses the line reader's separator positions, preserves quoted-string provenance through null matching, and adjusts separator positions when a UTF-8 BOM is removed.
Release note
Fix CSV FileScanner V2 parsing for enclosed fields and quoted null literals.
Check List (For Author)
CsvV2ReaderTest: 30 tests passedtest_local_tvf_csv_enclose_consistency: passed