Conversation
### What problem does this PR solve? Hive CSV tables created by Trino can store `separatorChar`, `quoteChar`, and `escapeChar` only in table parameters. The CSV scan-property extractor read only SerDe parameters, so custom CSV files were parsed with default delimiters, producing merged, shifted, or empty columns. Read CSV settings with the existing table-over-SerDe precedence and default fallback, including deriving quote trimming from the effective quote character. ### Release note Fix incorrect results when reading Hive CSV tables whose custom CSV settings are stored in table properties. ### Check List (For Author) - Test: - [x] Unit Test: six property tests and one scan-planning test covering both partitioned and unpartitioned tables. Six tests fail before the fix; all 42 targeted tests pass after it. - [x] Regression test added: four CSV file layouts, custom `s/q/e` settings, commas, escaped quotes, escape characters, empty strings, full-row reads, aggregates, and filters. Results are compared with the source data queried through Hive. - Full Hive connector suite: 469 tests passed; 1,239 tests passed including upstream modules. - FE Checkstyle and `git diff --check` passed. - Hive end-to-end execution remains pending: no reachable HiveServer2 in the local test configuration. - Behavior changed: - [x] Yes. CSV scans honor table parameters before SerDe parameters instead of silently using defaults. - Does this need documentation? - [x] No.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated static review of bdf7dc618a6c7bec518f9091302cefaafb5c9904 against 0aea7140de64a7162d0a0d764e3dc1db7f40dbae. Two P1 data-correctness findings are attached inline.
Checkpoint conclusions:
- Goal and proof: the ordinary ASCII table-only separator/quote/escape path is correctly propagated from HMS through the table handle and scan properties, but the two inline cases keep the change from fully matching the source reader.
- Focus and scope: no additional focus was supplied; all four changed files and the complete HMS-to-FE-to-thrift-to-BE path were reviewed. The table-level
line.delimexpansion is outside the stated three-key fix. - Concurrency: the change is per-scan immutable-map lookup; no new shared state, race, atomic, or lock-order issue.
- Lifecycle: no resource ownership, callback, cache retirement, or shutdown-order change.
- Configuration: no Doris runtime/configuration-reload surface changes; these values are remote HMS metadata.
- Compatibility and rolling upgrades: no SPI, thrift schema, symbol, or persisted-format change. The existing byte-sized quote/escape fields are the compatibility constraint called out inline.
- Parallel paths: synchronous/batched and partitioned/unpartitioned planning share the same extraction path; both V1 and V2 BE CSV readers were traced. The existing per-partition SerDe limitation predates this patch.
- Conditions and error handling: null/absent maps, table-only, SerDe-only, conflicts, per-key fallback, and ASCII defaults work. Empty, multi-character, and non-ASCII character cases need the validation/normalization described inline.
- Test coverage: the unit tests cover ordinary precedence and defaults, and the dynamic Hive suite is a coherent ASCII oracle for real partitioned and unpartitioned tables. It lacks negative record-delimiter and post-thrift character-contract coverage.
- Test outputs: the cross-engine JDBC results have stable ordering/cardinality by static inspection; no handwritten result file is involved.
- Observability: no new metrics or logging are needed once unsupported metadata fails loudly.
- Transactions and persistence: no Doris transaction, EditLog, replay, or persisted-state behavior changes.
- Data writes: production code changes only external-table scan planning; write atomicity and cleanup are not implicated.
- FE-to-BE propagation: the existing string delimiter and
i8quote/escape paths were checked in both readers; the latter is the second finding. - Performance: constant-time map lookups once per scan-node property construction; no material CPU, memory, I/O, or RPC regression found.
- Other issues: no additional distinct finding remained after two review rounds and duplicate fencing.
Verification status: static review only. Per the review contract, I did not run builds or tests. Checkstyle and lightweight CI checks are passing; FE UT, compile, and performance were still pending at submission time. The author reports connector/unit suites passing and explicitly says the Hive end-to-end run is pending; those claims were not independently executed here.
| String quoteChar = getParamOrDefault(params, QUOTE_CHAR, "\""); | ||
| getParamOrDefault(params, tableParams, SEPARATOR_CHAR, ",")); | ||
| result.put(ScanNodePropertyKeys.TEXT_LINE_DELIMITER, | ||
| getParamOrDefault(params, tableParams, LINE_DELIM, DEFAULT_LINE_DELIM)); |
There was a problem hiding this comment.
[P1] Keep table-level line.delim from changing record boundaries
The stated Trino layout has only separator/quote/escape in table parameters. Hive OpenCSVSerde never reads line.delim, and Hadoop TextInputFormat's actual record-delimiter setting is textinputformat.record.delimiter; however, this now forwards any unrelated table line.delim to BE, where both CSV readers split records on it. A newline-delimited table carrying TBLPROPERTIES ('line.delim'='|') will therefore return different rows in Doris than Hive. Please keep this lookup on the previous SerDe-only path (or the newline default), apply table precedence only to the three supported CSV character keys, and add a negative record-boundary case.
There was a problem hiding this comment.
Fixed in 9c7e82e. Table-over-SerDe lookup is now limited to separatorChar, quoteChar, and escapeChar. CSV line.delim retains its previous SerDe-only override and newline default.
Added property tests for ignored table line.delim values and preserved SerDe overrides, plus assertions after real scan-node construction and a Thrift round trip. The Hive regression suite now places pipes in payloads while setting table line.delim to |, across all four partitioned/unpartitioned layouts. Local unit/wire tests pass; Hive end-to-end execution remains pending because HiveServer2 is unavailable.
| getParamOrDefault(params, SEPARATOR_CHAR, ",")); | ||
| result.put(ScanNodePropertyKeys.TEXT_LINE_DELIMITER, getLineDelimiter(params)); | ||
| String quoteChar = getParamOrDefault(params, QUOTE_CHAR, "\""); | ||
| getParamOrDefault(params, tableParams, SEPARATOR_CHAR, ",")); |
There was a problem hiding this comment.
[P1] Normalize table CSV values before forwarding them
Hive OpenCSVSerde consumes the first Java character of each of these three properties and fails on an empty value, but this new table-level path forwards the raw strings: separatorChar='||' becomes a two-byte BE delimiter, empty quote/escape values are silently omitted downstream, and a one-character non-ASCII quote/escape such as é is reduced by getBytes()[0] to one byte in thrift i8. Those cases therefore still return wrong rows or silently differ from the source reader. Please normalize non-empty values to the first character, reject empty values, and either reject non-byte-representable quote/escape characters or extend the FE/BE contract; add coverage after thrift construction rather than only at the property-map layer.
There was a problem hiding this comment.
Fixed in 9c7e82e with a shared CSV-character resolver. It resolves precedence first, takes the first Java character as OpenCSVSerde does, rejects empty values and leading surrogate code units, and rejects non-ASCII quote/escape characters before the existing i8 fields can truncate them. Valid UTF-8 separators remain strings. Quote trimming is derived from the normalized character.
Replaced the incorrect empty-means-disabled test and added boundary tests for both property sources, numeric-looking values, precedence over invalid lower-priority values, and Unicode. Five FE Core tests call the real scan-node attribute builder and assert the deserialized Thrift payload. The Hive regression suite adds metadata-only multi-character changes and unsupported quote/escape errors. Empty metadata is tested at the unit boundary because Hive itself rejects it while validating ALTER TABLE.
Validation: 474 Hive connector tests passed (1,244 with upstream modules); all five FE Core wire tests and FE Checkstyle passed. The six new property-boundary tests failed before this follow-up fix. The Hive regression suite compiles, but its end-to-end assertions could not run without HiveServer2.
FE UT Coverage ReportIncrement line coverage `` 🎉 |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of exact head 9c7e82edec422fc6e438a139569a707ff3447c32. Requesting changes for two distinct Hive/OpenCSV compatibility defects: the explicit default-escape sentinel is sent to the BE with the wrong effective value, and the resolved separator/quote/escape tuple is not checked against OpenCSV's joint invariants. The two earlier inline threads at this head were treated as hard duplicate fences and are not repeated.
Critical checkpoint conclusions:
- Goal and scope: the focused five-file change correctly adds table-over-SerDe lookup, preserves the SerDe-only record delimiter, and covers the normal scan/wire path, but the goal is incomplete for the two accepted boundaries.
- Concurrency, lifecycle, and configuration: no shared mutable state, threads, locks, lifecycle ownership, static initialization, or dynamic configuration behavior is added.
- Error handling and conditions: planning exceptions propagate normally and the individual empty/representation checks are clear; the missing sentinel mapping and tuple validation are the blocking condition gaps.
- Compatibility, parallel paths, and FE-to-BE transport: the existing scan-property/Thrift fields are used by the common synchronous/batch planning path and both BE CSV readers. UTF-8 separators and ASCII quote/escape transport are otherwise sound, but both readers receive the two incorrect configurations described inline.
- Tests and results: connector, FE-core wire, and Hive regression coverage is structurally strong for precedence, normalization, partition layouts, and the prior fixes, but does not cover either accepted defect. Per the review contract, no build or test was run here; the PR also reports that the Hive end-to-end assertions were not executed because HiveServer2 was unavailable.
- Persistence, transactions, writes, and schema: not applicable to this scan-only change. The unsupported OpenCSV write path predates the PR and is not newly regressed.
- Performance, memory/nullability, and observability: work is constant per scan-property construction; no memory ownership, BE nullable, or new operational-observability concern was introduced.
User focus: no additional focus was provided. Review status: complete static review with two accepted inline findings.
| // BE's extra double-quote trimming is valid only for the effective double-quote enclosure. | ||
| result.put(ScanNodePropertyKeys.TEXT_TRIM_DOUBLE_QUOTES, String.valueOf("\"".equals(quoteChar))); | ||
| result.put(ScanNodePropertyKeys.TEXT_ESCAPE, getCsvCharacter(params, tableParams, ESCAPE_CHAR, '\\')); | ||
| result.put(ScanNodePropertyKeys.TEXT_NULL_FORMAT, ""); |
There was a problem hiding this comment.
[P1] Preserve Hive's explicit default-escape sentinel
Hive does not pass an explicit raw escapeChar='"' through to OpenCSV. OpenCSVSerde.newReader recognizes that writer-default value and selects the reader constructor whose parser escape is backslash. Here it reaches TEXT_ESCAPE as a double quote, so both BE readers use quote as the escape character; a valid file with a backslash-escaped embedded quote can therefore return a different value from Hive. Please map this raw sentinel to the reader-effective backslash before emitting the scan property, and cover the explicit table/SerDe value through the Thrift and Hive-read paths. This is separate from the earlier first-character/byte-representation thread.
There was a problem hiding this comment.
Addressed in 81d106e. The resolver now uses Hive's writer-default escape and maps a normalized double quote to the reader-effective backslash before validation and scan-property emission. Table/SerDe precedence still applies before interpreting the sentinel.
Added property tests for both sources, multi-character sentinel values, and precedence in both directions; FE tests verify the backslash after a real scan-node/Thrift round trip. A test using Hive 3.1.3's actual OpenCSVSerde checks backslash-escaped embedded quotes with both standard and custom quote characters. The Hive regression suite writes with an explicit backslash, switches only the metadata to the sentinel in each property source, and compares source, Hive CSV, and Doris results.
Validation: all 479 Hive connector tests (1,249 including upstream modules) and eight FE Core wire tests pass. The new property and Hive-reader tests reproduce the pre-fix failures. The HiveServer2 end-to-end suite compiles but remains pending because the local service is unavailable.
| result.put(ScanNodePropertyKeys.TEXT_LINE_DELIMITER, getLineDelimiter(params)); | ||
| String quoteChar = getParamOrDefault(params, QUOTE_CHAR, "\""); | ||
| getCsvCharacter(params, tableParams, SEPARATOR_CHAR, ',')); | ||
| // OpenCSVSerde does not use table-level line.delim to frame records. Preserve the existing |
There was a problem hiding this comment.
[P2] Validate the resolved characters as one OpenCSV configuration
Hive's OpenCSV parser rejects a NUL separator and any equal non-NUL pair among separator, quote, and escape, but these values are validated independently here. For example, table-level separatorChar='|' plus quoteChar='|' now reaches the BE and is parsed ambiguously even though Hive rejects the same table while constructing its reader. Please validate the three parser-effective characters together after applying the default-escape sentinel, and add conflict/NUL cases while keeping a raw double-quote quote/escape pair valid. This is distinct from the prior empty/non-ASCII validation thread.
There was a problem hiding this comment.
Addressed in 81d106e. Validation now operates on the complete parser-effective tuple after first-character normalization, property precedence, and default-escape sentinel conversion. A NUL separator or any equal active pair fails during planning. NUL quote/escape remain valid disabled settings, and a raw double-quote quote/escape pair stays valid because the reader-effective escape is backslash.
Added pairwise conflict tests from table, SerDe, and mixed sources, normalization-induced conflicts, sentinel-induced conflicts, and NUL acceptance/rejection tests. The new compatibility test uses Hive 3.1.3's actual reader as the oracle for 343 tuples across those three property sources. FE tests cover rejection before Thrift construction and zero-valued quote/escape bytes after serialization. The Hive regression suite also includes all three pairwise conflicts and a sentinel-induced conflict.
All 479 Hive connector tests (1,249 with upstream modules), including the actual Hive-reader comparisons, and all eight FE Core wire tests pass. End-to-end HiveServer2 execution remains pending locally.
TPC-H: Total hot run time: 16825 ms |
### What problem does this PR solve? Related PR: apache#68027 Problem Summary: An explicit double-quote escape property is Hive's writer-default sentinel, but its reader uses backslash for that value. Passing the raw sentinel to the BE corrupts escaped fields. Resolve the sentinel before emitting scan properties and reject NUL separators or duplicate active parser characters, while allowing disabled NUL quote/escape characters. ### Release note Honor Hive's explicit default CSV escape and reject invalid CSV character combinations before scanning. ### Check List (For Author) - Test: 479 Hive connector tests (1,249 including upstream modules), eight FE Core Thrift tests, real Hive reader comparisons for 343 character tuples with three property sources, FE Checkstyle, and Groovy compilation. HiveServer2 end-to-end execution remains pending. - Behavior changed: Yes. Match Hive's reader-effective escape and character constraints. - Does this need documentation: No.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of 81d106e4a9a0d53fc170203da13f0194831510db; no builds or tests were run, as required by the review task.
Requesting changes for three P2 Hive/OpenCSV row-semantics mismatches that affect both scanner implementations: leading-whitespace quote recognition, disabled-quote start-of-field escaping, and doubled enclosures when escape is disabled.
Checkpoint summary: the change is focused, and table/SerDe precedence, first-character normalization, the escape sentinel, effective-tuple validation, table-wide partition propagation, Thrift transport, test-only dependency packaging, and Surefire composition are otherwise coherent. No new concurrency, lifecycle, runtime-configuration, transaction/persistence, write-atomicity, observability, storage-format, FE/BE schema, or material performance risk was found. Tests cover property resolution, tuple acceptance, and wire transport broadly, but they do not execute these row states or deterministically cover both V1 and V2. There was no additional user-provided review focus.
| // Trino stores CSV settings in table parameters. Honor Hive's table-over-SerDe precedence | ||
| // so valid CSV files are not silently split with the default delimiter and quote characters. | ||
| String separator = getCsvCharacter(params, tableParams, SEPARATOR_CHAR, ','); | ||
| String quoteChar = getCsvCharacter(params, tableParams, QUOTE_CHAR, '"'); |
There was a problem hiding this comment.
[P2] Match OpenCSV's leading-whitespace quote handling
Forwarding the table-level quote still does not make these rows Hive-compatible because OpenCSV defaults ignoreLeadingWhiteSpace to true, while the shared Doris line reader recognizes an enclosure only as the literal first byte of a field. With separator |, quote q, escape e, and record x| qa|bq|c, Hive/OpenCSV discards the two spaces, enters quote state at q, and returns [x, a|b, c]. Both Doris readers see the first space, enter NORMAL state, and split on the | inside qa|bq, producing four fields. Please implement the OpenCSV leading-whitespace quote transition for this Hive CSV path and add a row-level oracle case under both scanner settings.
There was a problem hiding this comment.
Addressed in a101477. Both scanners now select a shared Hive OpenCSV parser with the full field-state transitions, including leading Unicode whitespace and embedded quotes. The 606-record corpus is generated by the actual Hive 3.1.3 SerDe and exercised through both BE readers.
| // so valid CSV files are not silently split with the default delimiter and quote characters. | ||
| String separator = getCsvCharacter(params, tableParams, SEPARATOR_CHAR, ','); | ||
| String quoteChar = getCsvCharacter(params, tableParams, QUOTE_CHAR, '"'); | ||
| String escapeChar = getCsvCharacter(params, tableParams, ESCAPE_CHAR, '"'); |
There was a problem hiding this comment.
[P2] Preserve quote doubling when escape is disabled
This accepts an active quote with escapeChar=NUL, but OpenCSV's doubled-quote rule is independent of the configured escape while both Doris value decoders disable all CSV unescaping when the escape byte is zero. With separator ,, quote q, escape NUL, and record qaqqbq,tail, Hive/OpenCSV collapses the middle qq and returns [aqb, tail]. Both Doris readers frame and trim the outer quotes correctly, then skip escape_string_for_csv, returning [aqqb, tail]. The tuple oracle only checks empty-record acceptance, so it misses the wrong value. Please preserve doubled-enclosure decoding with a NUL escape in both readers (or reject this unsupported tuple) and add a row-level oracle case under both scanner settings.
There was a problem hiding this comment.
Addressed in a101477. Doubled quotes are decoded independently of whether the escape character is NUL. Fields are decoded during OpenCSV parsing and materialized without a second generic CSV unescape pass. Disabled-escape cases are covered by the Hive-generated corpus in both scanners.
| if ("\0".equals(separator)) { | ||
| throw new DorisConnectorException("Invalid OpenCSVSerde property 'separatorChar': must not be NUL"); | ||
| } | ||
| // OpenCSV requires distinct active characters; NUL disables quote/escape and may be shared by both. |
There was a problem hiding this comment.
[P2] Cover disabled-quote rows, not only tuple acceptance
This accepts quoteChar=NUL with an active escape, but the BE unescape semantics do not match OpenCSV at the start of an unquoted field. For example, with separator ,, quote NUL, and escape e, Hive/OpenCSV reads eeabc,tail as abc,tail: its parser starts with inField=false, so both leading escapes are discarded. Both Doris readers instead use plain field splitting and escape_string_for_csv, which collapses ee to one e, returning eabc. Hive's writer itself emits eeabc for a value beginning with e under this tuple, so this is reachable without a malformed file. The 343-tuple oracle only compares acceptance and therefore misses the wrong row. Please align both BE readers with the OpenCSV field-state behavior (or reject tuples Doris cannot read equivalently) and add a row-level oracle case. This is distinct from the earlier validation thread: the tuple is accepted by both sides, but its data is decoded differently.
There was a problem hiding this comment.
Addressed in a101477. The Hive parser now follows OpenCSV's inField/inQuotes escape rules even when quoting is disabled, including removal of leading escapes. Binary and disabled-character records are compared with the actual Hive SerDe and tested through both BE readers.
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Sep 19th, 2026 8:24 AM. The selected account is excluded until 2026-09-19T08:24:00Z. Please trigger /review again; another configured account may be available. |
### What problem does this PR solve? Related PR: apache#68027 Problem Summary: The CSV metadata regression set line.delim before INSERT. Hive's text output format honors this property when writing records, so the fixture contained pipe-terminated records even though the test expected newline-delimited input. Set the property only after writing the files, and verify Hive can still read the original source rows before checking Doris. ### Release note None ### Check List (For Author) - Test: Reproduced the record-boundary difference with the actual Hive 3.1.3 text output format; Groovy suite compilation and git diff --check passed. Full external regression rerun pending. - Behavior changed: No production behavior change; correct regression data preparation. - Does this need documentation: No.
|
run buildall |
|
/review |
|
Fixed the external regression failure in 630c1e7. The CSV fixture set table-level Validation: a local probe using Hive 3.1.3's actual text output format reproduced |
There was a problem hiding this comment.
Static-only review of exact head 630c1e7714e3335996be605866ec02c8342961fb against base 0aea7140de64a7162d0a0d764e3dc1db7f40dbae. I found three additional P2 Hive/OpenCSV compatibility failures and left them inline. I did not repeat the existing current-head threads for leading whitespace before quotes, doubled quotes with disabled escape, or active escapes with disabled quote; those cases also remain applicable.
Critical-checkpoint conclusions:
- Goal and proof: The change correctly resolves table-over-SerDe character properties, first-character/default-escape normalization, tuple validation, and the existing FE-to-BE transport. It does not yet accomplish end-to-end Hive row compatibility: the three new counterexamples plus the three existing current-head threads produce different values, field counts, or record counts.
- Scope/focus: Production scope is localized to
HiveTextProperties; the POM and remaining files support tests. No unrelated SPI, engine, or catalog surface changed. - Concurrency: The resolver is stateless planning code with method-local values and immutable handle data. It adds no thread, shared mutable state, lock, or deadlock risk.
- Lifecycle: Metadata follows the existing handle/cache refresh lifecycle and survives handle rebuilding. No resource owner, close path, reference cycle, or static-initialization dependency was added.
- Configuration: No Doris configuration item or dynamic-update contract was introduced.
- Compatibility/rolling upgrade: No Thrift schema, connector SPI, symbol, or persisted format changed; existing optional string/i8 fields preserve mixed-version wire compatibility. The blockers are semantic compatibility with Hive/OpenCSV, not serialization compatibility.
- Parallel paths: Partitioned/unpartitioned and batch/non-batch planning share the resolved properties. Legacy CSV and FileScannerV2 both consume the same tuple and share the failing framing/decoding mechanisms, so neither is an unaffected fallback.
- Conditions/errors: Empty, surrogate, non-ASCII i8, NUL-separator, and active-character conflicts now fail loudly after effective precedence and sentinel conversion. The remaining failures are downstream parser-state mismatches, not silent FE fallback.
- Tests/results: The property, provider, and Thrift tests are focused and their assertions are correct for resolution/transport. The 343-tuple oracle checks only empty-record acceptance, the regression covers one ordinary active tuple without deterministically running both scanner modes, and the reported HiveServer2 end-to-end run remains pending. Value/record differential tests are required for all concrete failures. No build or test was run during this review, as required by the review environment.
- Observability: No new distributed or long-lived operation needs logs or metrics; actionable planning exceptions and existing scan profiles are sufficient.
- Persistence/transactions/data writes: Doris persistence, EditLog, transaction, and production write paths are untouched. Hive writes are regression-fixture setup only.
- FE-to-BE variables: The change reuses existing fields, and I traced table metadata through
HiveTableHandle,HiveScanPlanProvider,PluginDrivenScanNode, Thrift, and both readers. Transport is sound, including explicit zero bytes and UTF-8 separators. - Performance: Resolution is bounded map lookup and constant-time validation per scan-property build, with no material row-path, memory, or I/O cost.
- Other/user focus: No separate user focus was supplied. Packaging remains test-only, and no additional security, nullability, atomicity, or compatibility issue survived the final sweep.
The review completed two bounded rounds; every second-round reviewer returned NO_NEW_VALUABLE_FINDINGS, all candidates were adjudicated, and the live head/base and duplicate fence were refreshed immediately before submission.
| // Trino stores CSV settings in table parameters. Honor Hive's table-over-SerDe precedence | ||
| // so valid CSV files are not silently split with the default delimiter and quote characters. | ||
| String separator = getCsvCharacter(params, tableParams, SEPARATOR_CHAR, ','); | ||
| String quoteChar = getCsvCharacter(params, tableParams, QUOTE_CHAR, '"'); |
There was a problem hiding this comment.
[P2] Preserve Hive's physical record boundaries for custom quotes
With table-only separatorChar='|', quoteChar='q', and escapeChar='e', consider file bytes qleft\nrightq|tail\n. Hive's TextInputFormat gives the SerDe each physical line separately; OpenCSV reaches EOF with an unmatched pending quote on each call, so Hive exposes two null-valued rows. Once this assignment forwards q, both Doris scanner modes use EncloseCsvLineReaderCtx, which reads past the first newline until the second-line q and instead returns one row ['left\nright', 'tail']. This is separate from the resolved line.delim issue because the delimiter stays newline; activating the quote changes record ownership. Please preserve Hive's physical-line boundary (or reject this unsupported tuple) and cover the case with both scanner settings.
There was a problem hiding this comment.
Addressed in a101477. Hive CSV uses Hadoop-style physical LF/CRLF/CR records instead of quote-dependent line framing. An unmatched quote drops the pending field while preserving completed fields from that record. Both readers are tested for physical boundaries, counts, projections, split offsets, and empty records; the external regression explicitly selects both scanner modes.
| // Trino stores CSV settings in table parameters. Honor Hive's table-over-SerDe precedence | ||
| // so valid CSV files are not silently split with the default delimiter and quote characters. | ||
| String separator = getCsvCharacter(params, tableParams, SEPARATOR_CHAR, ','); | ||
| String quoteChar = getCsvCharacter(params, tableParams, QUOTE_CHAR, '"'); |
There was a problem hiding this comment.
[P2] Match OpenCSV's embedded-quote state transition
This still diverges after a non-whitespace prefix, independently of the existing leading-whitespace thread. With table-only separatorChar='|', quoteChar='q', escapeChar='e', the row abcqleft|rightq|tail is parsed by OpenCSV as ['abcqleft|right', 'tail', NULL]: the first q is retained but also enters quoted state, so the enclosed separator is data. Doris sees the initial a, moves permanently to NORMAL, and both scanner modes split both separators into ['abcqleft', 'rightq', 'tail']. A whitespace-only correction will not cover this branch; the Hive CSV framing path needs OpenCSV's full field quote transitions, with row-level tests under both scanners.
There was a problem hiding this comment.
Addressed in a101477. The shared Hive parser implements the complete embedded-quote transition, including the whole-record UTF-16 position rule and retention of the embedded quote when appropriate. It also applies OpenCSV escape semantics to the resulting field. Coverage uses actual Hive-generated row expectations and both real reader implementations, rather than only character-tuple acceptance.
| // so valid CSV files are not silently split with the default delimiter and quote characters. | ||
| String separator = getCsvCharacter(params, tableParams, SEPARATOR_CHAR, ','); | ||
| String quoteChar = getCsvCharacter(params, tableParams, QUOTE_CHAR, '"'); | ||
| String escapeChar = getCsvCharacter(params, tableParams, ESCAPE_CHAR, '"'); |
There was a problem hiding this comment.
[P2] Handle literal NUL consistently when quote and escape are disabled
The accepted tuple quoteChar=NUL, escapeChar=NUL disagrees on writer-reachable data. For a\0b,tail, OpenCSV 2.3 still takes its c == escape branch for the literal NUL and drops it, returning ['ab', 'tail']; its writer emits that NUL unchanged when both features are disabled. Doris selects plain splitting and skips CSV unescaping when both fields are zero, so both scanner modes preserve ['a\0b', 'tail']. This is distinct from the existing one-feature-disabled threads. Please either reproduce OpenCSV's literal-NUL behavior or reject the tuple as unsupported, and add a binary row-level oracle.
There was a problem hiding this comment.
Addressed in a101477. NUL participates in OpenCSV's escape-first branch even when both quote and escape are disabled. The shared corpus stores binary inputs and expected fields as hex, distinguishing NULL from empty strings. Both scanners consume that corpus, and the regression creates raw TEXTFILE fixtures containing literal NUL bytes.
TPC-H: Total hot run time: 17216 ms |
TPC-DS: Total hot run time: 83099 ms |
ClickBench: Total hot run time: 15.05 s |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Problem Summary: Hive CSV character properties alone do not select OpenCSV field states. Generic parsing can shift embedded-quote fields, merge physical records, and misdecode disabled escape/quote settings. Add an explicit Hive CSV scan attribute and a shared parser for both readers, with Hive-generated record expectations and deterministic scanner coverage. ### Release note Fix Hive OpenCSV row decoding, physical record boundaries, and missing fields. ### Check List (For Author) - Test: 480 Hive connector tests, 9 FE Core tests, 44 BE tests under ASAN; parser corpus under ASAN/UBSAN; Groovy compilation, FE Checkstyle, clang-format 16, and header hygiene. External regression pending CI. - Behavior changed: Yes, supported Hive CSV scans use OpenCSVSerde semantics. - Does this need documentation: No.
Gabriel39
changed the title
|
run buildall |
|
/review |
### What problem does this PR solve? Related PR: apache#68027 Problem Summary: License Eye cannot infer a comment style for the generated TSV oracle. The FE test compares every line with regenerated Hive output and the BE tests parse every line as a record. Follow existing golden-fixture exclusions with a single documented path entry so the data stays unchanged. ### Release note None ### Check List (For Author) - Test: License Eye v0.8.0 header checks; git diff --check - Behavior changed: No - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Static review of exact head 15decc03b6957fe4212b7de85fd8ee913d888810 against target snapshot 0aea7140de64a7162d0a0d764e3dc1db7f40dbae.
Requesting changes for two P1 compatibility failures: the new public connector property remains under API major 8, so mismatched Hive plugin/FE pairs silently lose OpenCSV mode; and the new optional Thrift flag has no old-BE fence, so a rolling-upgrade backend silently selects generic CSV semantics. Both produce data-dependent wrong query results rather than a compatibility error.
Critical checkpoints:
- Goal and proof: for a current plugin, FE, and BE, the change now matches Hive/OpenCSV field states and physical records across both scanners. The Hive-generated oracle, BE reader tests, FE property/wire tests, and external regression cover the intended behavior, but they do not make the two mixed-version deployments safe.
- Scope/focus: all 21 authoritative changed paths and their upstream/downstream call chains were reviewed. The implementation is otherwise focused; no additional user focus was provided.
- Concurrency and lifecycle: parser/reader state is per reader; no new shared concurrent state or lock path is introduced. Parser-owned decoded slices are consumed before reuse, and BOM/header/split/EOF/count lifecycles were traced without another issue.
- Configuration and conditions: no runtime configuration item is added. OpenCSV tuple normalization and special quote/escape/NUL conditions are covered by the existing resolved threads and were not duplicated here.
- Compatibility and FE-BE transport: current-version property propagation reaches the shared scan-level attributes and both V1/V2 readers, but the plugin/FE API-major gate and FE/old-BE execution boundary are both missing required fences; see the inline comments.
- Parallel paths: both scanner implementations, projected/missing/empty/null materialization, batch/streaming splits, compression, and physical LF/CRLF/CR ownership were checked. No additional divergence survived the two-round review.
- Tests/results: the added unit/oracle/regression coverage is substantial and discoverable. This reviewer did not run builds or tests because the review runner contract explicitly prohibits them, so this conclusion is static-only and does not independently verify the author-reported results.
- Observability, persistence, and writes: no new persistent state, transaction/data-write path, or observability requirement is introduced.
- Performance: decoded-buffer reuse and bounded requested-field parsing avoid a distinct material regression; no actionable performance issue was found.
Review status: complete static review after two rounds; all candidates were deduplicated and adjudicated, and the second-round normal and risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS.
| public static final String TEXT_ENCLOSE = TEXT_PROPERTY_PREFIX + "enclose"; | ||
|
|
||
| /** {@code "true"} for Hive OpenCSV field states and physical record boundaries. */ | ||
| public static final String TEXT_HIVE_OPEN_CSV = TEXT_PROPERTY_PREFIX + "hive_open_csv"; |
There was a problem hiding this comment.
[P1] Version the connector SPI property addition
This is a new engine-read member of the shared connector SPI, but the connector API and its pinned test remain at 8.0 even though this repository defines every SPI-surface addition as a major change. Because this String constant is compile-time-inlined, a new API-8 Hive plugin still loads on an old API-8 FE, emits the literal key, and the old FE silently ignores it; the reverse pairing never emits it. Both paths fall back to generic CSV semantics instead of OpenCSV. Please bump the connector API major/update its pin and extend the frozen surface/baseline to include these public engine-read property keys so this protocol change cannot bypass the version gate.
There was a problem hiding this comment.
Addressed in 3cb2835. The connector API is now 9.0, with its pinned version updated in the same commit. ConnectorPluginSurfaceTest records every public ScanNodePropertyKeys field by name, type, and literal value, so an inlined key/value change becomes a visible API-surface change. Both baselines were regenerated; the metadata-method baseline remains identical.
The actual plugin directory loader now rejects an API 8.0 probe jar on this FE; the existing matching-version and other-major tests also pass. Validation: all 142 connector SPI tests, 480 Hive connector tests, and 34 FE Core tests passed. The old-version loader and version/surface checks reproduced the failures before the fix.
| 13: optional bool openx_json_ignore_malformed = false; | ||
|
|
||
| // Hive OpenCSVSerde has different field states and physical record boundaries from load CSV. | ||
| 14: optional bool hive_open_csv = false; |
There was a problem hiding this comment.
[P1] Fence this semantic flag from old backends
An older BE legally ignores unknown optional field 14, so it sees the default false and runs the generic CSV reader even though the new FE promised OpenCSVSerde semantics. That fallback is observably different for records already covered by this PR (for example custom quotes/leading whitespace), yet this change neither advances/checks a minimum BE execution version nor marks the Hive scan with REQUIRED_CURRENT_BACKEND_SEMANTICS. During a rolling upgrade, placement can therefore change query results instead of failing. Please gate OpenCSV scans on a BE version that implements this field (including the smooth-upgrade-source fence) and add an absent-field/mixed-version test.
There was a problem hiding this comment.
Addressed in 3cb2835. FE and BE now define OpenCSV support at execution version 15. The real scan attribute builder rejects OpenCSV when the configured query-wide execution version is below 15, and Hive emits REQUIRED_CURRENT_BACKEND_SEMANTICS so the existing scheduler fence rejects eligible smooth-upgrade source backends even at version 15.
Added FE tests for version 14 rejection, version 15 acceptance, a mixed candidate list containing a smooth-upgrade source, and the absent-flag wire contract. Both actual BE scanners additionally verify that an absent flag retains legacy decoding while the same bytes with the flag use OpenCSV decoding. The BE version manager accepts both legacy version 14 and the new version 15. Validation: 34 FE Core tests and 47 BE tests under ASAN passed; the missing FE fences and unsupported BE version were reproduced before the fix.
### What problem does this PR solve? Related PR: apache#68027 Problem Summary: Inlined SPI property keys and optional Thrift fields can be ignored by older components, silently restoring generic CSV decoding. Bump the connector API to 9.0, freeze scan property names and values in its surface baseline, and require backend execution version 15 for OpenCSV scans. Mark those scans for the existing smooth-upgrade-source fence. Preserve legacy decoding when the semantic flag is absent. ### Release note Hive OpenCSV scans require backend execution version 15 or newer. Connector plugins must use API 9.0. ### Check List (For Author) - Test: Connector SPI and Hive suites, FE scan and plugin compatibility tests, ASAN CSV reader tests, FE Checkstyle, clang-format 16, header hygiene, License Eye, and git diff --check. Clang-tidy is blocked by an existing unmatched NOLINTEND in core/types.h; the version manager has no diagnostics. - Behavior changed: Yes, reject incompatible OpenCSV execution during upgrades. - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Static review of exact head 3cb283539ef1e85ff10aa22a9d9ff58a6043fe35 against target snapshot 0aea7140de64a7162d0a0d764e3dc1db7f40dbae. Requesting changes for one P1 BE memory-safety issue: the new parser can allocate and retain a whole decoded-row copy without Doris allocator prechecks while the raw row and output coexist. Existing review threads were treated as hard duplicate fences and were not repeated.
Critical-checkpoint conclusions:
- Goal and proof: the change implements Hive OpenCSVSerde character, field-state, physical-record, projection, and missing-versus-empty semantics across both CSV scanners. The Hive-generated oracle plus FE, BE, and regression tests are broad, but the accepted wide-record allocation path remains unsafe, and no test proves controlled failure/release under a query memory limit.
- Focus: the 28 changed files are coherently scoped to property extraction, FE-to-BE transport/version fencing, shared parsing/record handling, and coverage.
- Concurrency: parser state is reader-local and scheduling/version state uses existing immutable paths; no new shared mutable state, lock, atomic, or deadlock issue was found.
- Lifecycle and static initialization: each range reader owns its parser and consumes decoded slices before the next parse. There is no new cross-TU static dependency or reference cycle; the blocking lifecycle issue is retained high-water scratch capacity until range-reader destruction.
- Configuration: no new runtime configuration item is added. The existing UTF-8-validation option is propagated through the established scan path, so no dynamic-update gap was found.
- Compatibility and rolling upgrades: connector API 9, execution version 15, optional Thrift defaulting, FE construction checks, and the smooth-upgrade-source scheduling fence cover old/new plugin, FE, and BE combinations without silent fallback.
- Parallel paths: V1 and FileScannerV2 both use the shared parser and physical-record context, including projection and count paths; no missed parallel implementation remained.
- Conditions and errors: tuple validation, NUL/disabled characters, default-escape conversion, execution-version guards, missing fields, and UTF-8 modes were traced and match their stated invariants. Errors propagate through existing Status/exception boundaries with adequate context.
- Test coverage: parser-oracle prefixes, dense/reordered projections, missing versus empty values, LF/CRLF/CR, BOM, split offsets, counts, tuple validation, wire defaults, API loading, and version fences are covered. The external Hive regression is end-to-end in structure, but this review did not execute it.
- Test results: added expectations and generated oracle rows are internally consistent by static inspection. Per the review contract, I ran no builds or tests; current CI has format, checkstyle, license, dependency, secrets, and Cloud UT passing, while BE UT, FE UT, compile, and performance are still pending.
- Observability: no new distributed operation or metric is required; existing scan/planning errors and profiles are adequate once the memory path fails before allocation.
- Transactions and persistence: no Doris transaction, EditLog, replay, master-failover, or persisted-format behavior is changed.
- Data writes: production changes are read-side only, so write atomicity and crash cleanup are not implicated.
- FE-to-BE variables: the new OpenCSV flag and character properties reach the common scan attributes, optional Thrift field, and both BE readers; all identified send and receive paths are updated.
- Performance and memory: field parsing is prefix-bounded and otherwise linear, but the accepted decoded-row scratch creates an additional row-sized allocation, lacks allocator prechecks, and retains outlier capacity. No other material CPU, I/O, or RPC regression survived review.
- Other issues: the pre-existing multi-header split limitation and raw-byte behavior when UTF-8 validation is disabled are not introduced defects. No additional issue remained after two bounded rounds, and every Round 2 reviewer returned
NO_NEW_VALUABLE_FINDINGS.
User focus: no additional focus was supplied. Completion status: complete static review with one accepted inline finding; no builds, tests, or source edits were performed.
Restore the legacy format readers, relocate CSV helpers and tests to format_v2, and avoid NUL metadata in PostgreSQL-backed Hive regression fixtures.
|
run buildall |
|
/review |
|
Scoped the CSV reader fix to FileScannerV2 and restored Also corrected the CSV regression fixture: Hive decodes the disabled-character SQL literal to NUL, which a PostgreSQL-backed metastore cannot persist in text properties. The integration fixture now uses four valid dialects. Disabled quote/escape settings and binary NUL data remain covered by the Hive oracle and V2 unit tests. Validation: 43 BE tests passed under ASAN, 480 Hive connector tests passed, Groovy compilation and FE Checkstyle passed, and all affected C++ files passed clang-format 16. License Eye and header hygiene passed. A real Hive SQL-decoding/PostgreSQL probe reproduced the original metadata failure and successfully persisted all 12 revised property values. The V2 parser and line helper are clang-tidy clean; existing reader/header diagnostics still prevent a clean repository-wide result. Full external regression validation awaits the new CI run. |
Gabriel39
changed the title
There was a problem hiding this comment.
Static review at exact head f5efe3445cf31d704039d337e6485b54719b13ba: requesting changes for two newly substantiated issues.
- The OpenCSV semantic flag can still execute through the legacy scanner when
enable_file_scanner_v2=false, silently changing valid Hive rows. - The new physical-line context rescans accumulated compressed records from byte zero, making large valid records quadratic to frame.
Critical checkpoints:
- Goal/data correctness: the V2 parser and table-over-SerDe property path otherwise match the inspected Hive/OpenCSV cases; the goal is incomplete because the parallel V1 path can bypass them.
- Scope/compatibility/configuration: connector API 9, the optional Thrift field, execution version 15, and smooth-upgrade fencing are coherent; the existing session scanner selector is the remaining compatibility gap.
- Concurrency/lifecycle/null/error handling: state is per reader with no new shared locks or threads; returned slices are consumed before parser reuse; missing versus empty/NULL handling and fail-loud version checks are coherent.
- Performance/memory: the quadratic compressed-record scan is newly reported. The existing allocator/retained-scratch P1 at discussion_r4025769228 remains open and was not duplicated.
- Tests/results: the deterministic Hive oracle, FE wire/SPI tests, direct V2 BE tests, and regression are broad, but they force or directly construct V2 and use uncompressed input, so they miss both findings. The external end-to-end rerun remains pending per the PR description.
- Persistence/transactions/writes/observability: this is a read-only scan change; no EditLog, transaction, or data-write path is modified, and no additional observability gap was found.
- FE-to-BE contract: the new property reaches Thrift and FileScannerV2, but not the legacy reader selected by the session option.
Validation was static-only as required; no builds or tests were run. No additional user focus was specified.
| throw new UserException("Hive OpenCSVSerde requires backend execution version " | ||
| + Config.HIVE_OPEN_CSV_MIN_BE_EXEC_VERSION + " or newer during rolling upgrade"); | ||
| } | ||
| attrs.setHiveOpenCsv(true); |
There was a problem hiding this comment.
[P1] Require FileScannerV2 for this semantic flag. This is distinct from the old-BE fence and earlier parser-state threads: a current version-15 BE receives hive_open_csv, but FileScanLocalState::should_use_file_scanner_v2() still honors enable_file_scanner_v2=false and routes the scan through legacy FileScanner, whose CSV reader never consumes the flag. That is a silent correctness fallback: with the default read_csv_empty_line_as_null=false, a blank physical record is skipped by V1, while Hive/OpenCSVSerde (and this V2 implementation) returns an all-NULL row. The regression's checkV2 helper forces V2 on, so it cannot catch this. Please force V2 for OpenCSV scans (as the ADBC path does) or reject planning when V2 is disabled.
| namespace doris::format::csv { | ||
|
|
||
| const uint8_t* HiveCsvLineReaderCtx::read_line(const uint8_t* start, size_t len) { | ||
| for (size_t i = 0; i < len; ++i) { |
There was a problem hiding this comment.
[P1] Resume delimiter search after compressed refills. NewPlainTextLineReader calls this context repeatedly with the entire accumulated output buffer until a delimiter appears, but this loop restarts at byte 0 each time. Compressed input is refilled in 2 MiB chunks, so a valid incompressible N-byte physical record is searched over prefixes of roughly 2 MiB, 4 MiB, ... N (about 256 GiB of byte checks for a 1 GiB record) before the CSV parser scans it again. The previous enclosure-aware context retained _idx, and the added tests are all uncompressed. Please retain a per-record search offset, rechecking only a terminal CR for CRLF lookahead, and reset it in refresh().
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 16940 ms |
TPC-DS: Total hot run time: 82872 ms |
ClickBench: Total hot run time: 14.89 s |
2 participants
What problem does this PR solve?
Hive CSV tables created by Trino can store character settings only in table parameters. Doris used SerDe parameters and a generic CSV parser, producing merged, shifted, or empty data columns even when partition values were correct.
Resolve table-over-SerDe character precedence, first-Java-character normalization, defaults, and Hive's double-quote escape sentinel. Validate the effective character tuple and reject settings that cannot be represented by the existing character fields.
Implement OpenCSV row semantics in FileScannerV2. The parser and physical-record context live under
be/src/format_v2/delimited_text/, with tests underbe/test/format_v2/delimited_text/. The legacybe/src/format/directory is unchanged from the PR base. Match OpenCSV 2.3 field transitions, including leading whitespace, embedded and doubled quotes, disabled quote/escape settings, and literal NUL bytes. Preserve Hadoop physical records (LF, CRLF, and CR), completed fields before an unmatched quote, missing-field NULLs, explicit empty strings, and file-start BOM handling. Decode each field once and bound delimiter metadata by the requested column prefix. Neither table nor SerDeline.delimoverrides CSV input framing.Version the contract at both boundaries: connector API 9.0 freezes the public scan-property names, types, and values; OpenCSV scans require query-wide BE execution version 15 or newer and exclude smooth-upgrade source backends. Requests without the optional semantic flag retain generic CSV decoding in FileScannerV2.
The external regression uses Hive-compatible, non-NUL character properties because PostgreSQL-backed Hive metastores cannot store NUL in text metadata. Disabled quote/escape combinations remain covered by the Hive oracle and V2 unit tests; raw file records still include binary NULs.
Release note
Fix incorrect results when FileScannerV2 reads Hive OpenCSV tables with custom character properties and OpenCSV-specific quoting or escaping. Reject unsupported character metadata explicitly. OpenCSV scans require BE execution version 15 or newer; connector plugins must use API 9.0.
Check List (For Author)
NOLINTENDincore/types.hand existing reader diagnostics. The V2 parser and physical-record helper have no clang-tidy diagnostics.