github-actions[bot] commented on code in PR #65501:
URL: https://github.com/apache/doris/pull/65501#discussion_r3566380381
##########
be/src/format/file_reader/new_plain_text_line_reader.cpp:
##########
@@ -46,6 +46,23 @@
namespace doris {
const uint8_t* EncloseCsvLineReaderCtx::read_line_impl(const uint8_t* start,
const size_t length) {
+ if (_skip_utf8_bom && !_first_record_prefix_checked && _idx == 0) {
+ constexpr uint8_t UTF8_BOM[] = {0xEF, 0xBB, 0xBF};
+ constexpr size_t UTF8_BOM_SIZE = sizeof(UTF8_BOM);
+ const size_t prefix_size = std::min(length, UTF8_BOM_SIZE);
+ if (std::memcmp(start, UTF8_BOM, prefix_size) != 0) {
+ _first_record_prefix_checked = true;
+ } else if (length < UTF8_BOM_SIZE) {
+ // The input buffer can end inside the BOM. Wait for the remaining
prefix bytes instead
+ // of feeding a partial BOM into START, which would permanently
select NORMAL state.
+ return nullptr;
+ } else {
+ // Keep offsets relative to the original buffer. CsvReader removes
these three bytes
+ // from the returned line and shifts separator positions by the
same amount.
+ _idx = UTF8_BOM_SIZE;
+ _first_record_prefix_checked = true;
+ }
+ }
// Avoid part bytes of the multi-char column separator have already been
parsed,
// causing parse column separator error.
if (_state.curr_state == ReaderState::NORMAL ||
Review Comment:
The refill backtracking here can re-scan inside a separator that was already
accepted on the previous `read_line()` call for the same logical record.
`NewPlainTextLineReader::read_line()` calls `refresh()` once, extends the
output buffer when no line delimiter is found, and calls this context again
with the existing `_idx` and `column_sep_positions()`. If a multi-character
separator was just recorded and the buffer ended shortly into the next field,
this unconditional rewind by `_column_sep_len - 1` can move `_idx` back into
that accepted separator. A long row using `column_separator = "|||"` with bytes
around the refill boundary like `...|||||b\n` can therefore record overlapping
separator positions; the new v2 splitter then trusts those positions and hits
`DORIS_CHECK_LE(value_start, separator_position)` in
`CsvReader::_split_line()`. Please clamp the rewind so it never searches before
the end of the last accepted separator, or otherwise track only uncommitted
suffix bytes for p
artial-separator matching, and add a v2 test with an enclosed CSV row larger
than one line-reader output buffer and a multi-character separator.
##########
regression-test/suites/external_table_p0/tvf/test_local_tvf_csv_enclose_consistency.groovy:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_local_tvf_csv_enclose_consistency", "p0,external") {
+ def backends = sql "show backends"
+ assertTrue(backends.size() > 0)
+ def beId = backends[0][0]
+ def dataPath = context.config.dataPath + "/external_table_p0/tvf"
+ def filePath = dataPath
+
+ def files = [
+ "csv_enclose_state.csv",
+ "csv_matching_escape_enclose.csv",
+ "csv_quoted_null.csv"
+ ]
+ if (backends.size() > 1) {
Review Comment:
This only copies the new CSV fixtures when there is more than one backend.
In a single-BE regression run where the BE is remote or otherwise does not
share the regression client's `context.config.dataPath`, `filePath` stays as
the client-side regression data directory and the later `local(...,
"backend_id" = "${beId}")` calls ask the BE to open files that were never
copied there. Nearby local TVF suites copy their fixtures to each BE
unconditionally before using `local()` with a pinned backend id. Please copy
these files, or the containing fixture directory, to a BE-local path
unconditionally and use that path for both single-BE and multi-BE runs.
##########
be/src/format_v2/delimited_text/csv_reader.cpp:
##########
@@ -174,77 +173,40 @@ void CsvReader::_split_line(const Slice& line) {
return;
}
- // The text line reader is responsible for split boundaries and multi-line
quoted fields.
- // Field slicing still happens here because FileScannerV2 asks columns by
file-local id, so we
- // must be able to materialize only the requested CSV ordinals without
building a row object.
- // Example: for `1,"a,b",10` and column separator `,`, this loop returns
three slices:
- // `1`, `a,b`, and `10`; the comma inside quotes does not create an extra
field.
- bool in_quote = false;
- bool escaped = false;
- size_t start = 0;
- size_t i = 0;
- while (i < line.size) {
- const char ch = line.data[i];
- if (_enclose != 0) {
- if (escaped) {
- escaped = false;
- ++i;
- continue;
- }
- if (_escape != 0 && ch == _escape) {
- escaped = true;
- ++i;
- continue;
- }
- if (ch == _enclose) {
- if (in_quote && i + 1 < line.size && line.data[i + 1] ==
_enclose) {
- i += 2;
- continue;
- }
- in_quote = !in_quote;
- ++i;
- continue;
- }
+ const auto append_value = [&](size_t value_start, size_t value_len) {
+ while (_trim_tailing_spaces && value_len > 0 &&
+ line.data[value_start + value_len - 1] == ' ') {
+ --value_len;
}
- if (!in_quote && starts_with_at(line, i, _value_separator)) {
- size_t value_start = start;
- size_t value_len = i - start;
- while (_trim_tailing_spaces && value_len > 0 &&
- line.data[value_start + value_len - 1] == ' ') {
- --value_len;
- }
- if (_trim_double_quotes && value_len > 1 && line.data[value_start]
== '"' &&
- line.data[value_start + value_len - 1] == '"') {
- ++value_start;
- value_len -= 2;
- } else if (_enclose != 0 && value_len > 1 &&
line.data[value_start] == _enclose &&
- line.data[value_start + value_len - 1] == _enclose) {
- ++value_start;
- value_len -= 2;
- }
- _split_values.emplace_back(line.data + value_start, value_len);
- i += _value_separator.size();
- start = i;
- continue;
+ if (_enclose != 0 && value_len > 1 && line.data[value_start] ==
_enclose &&
+ line.data[value_start + value_len - 1] == _enclose) {
+ ++value_start;
+ value_len -= 2;
}
- ++i;
- }
+ _split_values.emplace_back(line.data + value_start, value_len);
+ };
- size_t value_start = start;
- size_t value_len = line.size - start;
- while (_trim_tailing_spaces && value_len > 0 && line.data[value_start +
value_len - 1] == ' ') {
- --value_len;
- }
- if (_trim_double_quotes && value_len > 1 && line.data[value_start] == '"'
&&
- line.data[value_start + value_len - 1] == '"') {
- ++value_start;
- value_len -= 2;
- } else if (_enclose != 0 && value_len > 1 && line.data[value_start] ==
_enclose &&
- line.data[value_start + value_len - 1] == _enclose) {
- ++value_start;
- value_len -= 2;
+ size_t value_start = 0;
+ if (_enclose_reader_ctx != nullptr) {
+ for (const size_t separator_position :
_enclose_reader_ctx->column_sep_positions()) {
Review Comment:
`_enclose_reader_ctx->column_sep_positions()` can now contain separators
that were found in the delimiter bytes, but this function splits a
delimiter-stripped `line`. For example, with `enclose` set, `line_delimiter =
"\n"`, `column_separator = "\n"`, and data `a\n`,
`EncloseCsvLineReaderCtx::update_reading_bound()` scans through
`line_delimiter_length()` and records separator offset 1 at the newline.
`read_line()` returns a slice of size 1 (`"a"`), this loop accepts
`separator_position == line.size`, then advances `value_start` to 2 and the
final `DORIS_CHECK_LE(value_start, line.size)` aborts the BE. The same can
happen for multi-byte separators that overlap the delimiter, such as
`column_separator = "|\n"`. Please either reject separator/delimiter equality
or overlap before scan execution, or ensure the enclosed line reader only
records separator positions that lie wholly inside the returned logical line
slice, and add v2 coverage for this case.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
