From a73aac51567d6029a2f47a67ebc7769f3d5d5c5b Mon Sep 17 00:00:00 2001 From: azimafroozeh Date: Mon, 28 Jul 2025 15:44:13 +0200 Subject: [PATCH 1/3] add tests --- test/src/unit_tests/CMakeLists.txt | 1 + .../csv_parser_trailing_delimiter_test.cpp | 75 +++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 test/src/unit_tests/csv_parser_trailing_delimiter_test.cpp diff --git a/test/src/unit_tests/CMakeLists.txt b/test/src/unit_tests/CMakeLists.txt index ff9ab436..efd3daa3 100644 --- a/test/src/unit_tests/CMakeLists.txt +++ b/test/src/unit_tests/CMakeLists.txt @@ -1,4 +1,5 @@ add_executable(unit_test + csv_parser_trailing_delimiter_test.cpp csv_reader_test.cpp double_test.cpp json_test.cpp diff --git a/test/src/unit_tests/csv_parser_trailing_delimiter_test.cpp b/test/src/unit_tests/csv_parser_trailing_delimiter_test.cpp new file mode 100644 index 00000000..48a8e62d --- /dev/null +++ b/test/src/unit_tests/csv_parser_trailing_delimiter_test.cpp @@ -0,0 +1,75 @@ +#include "fls/common/alias.hpp" +#include "fls/common/assert.hpp" +#include "fls/csv/csv-parser/parser.hpp" +#include "fls/csv/csv.hpp" +#include "gtest/gtest.h" +#include + +namespace fastlanes { + +std::vector scanTokens(aria::csv::CsvParser& p, size_t safety_limit = 10000) { + std::vector out; + size_t steps = 0; + for (;;) { + if (++steps > safety_limit) { + // If we ever hit this, the parser is stuck (buggy behavior). + // This makes the test fail fast instead of hanging. + out.push_back(""); + break; + } + aria::csv::Field f = p.next_field(); + if (f.type == aria::csv::FieldType::CSV_END) { + out.emplace_back("CSV_END"); + break; + } else if (f.type == aria::csv::FieldType::ROW_END) { + out.emplace_back("ROW_END"); + } else { + out.emplace_back(*f.data); + } + } + return out; +} + +} // namespace fastlanes + +// Bring types & helper into global scope for the TESTs: +using aria::csv::CsvParser; +using fastlanes::scanTokens; + +TEST(CsvParserBug, TrailingDelimiterLF) { + // A single row that ends with a delimiter before '\n' → last field is empty. + const std::string csv = "001|2014-08-24 18:00:00|3|0|21|19|0|\n"; + + std::istringstream iss(csv); + CsvParser parser(iss); + parser.delimiter('|'); // important for this dataset + + auto tokens = scanTokens(parser); + + // With the bug: tokens ends with many "ROW_END" and finally "" + // With the fix: we should see fields, then one empty field (""), one ROW_END, then CSV_END. + std::vector expected = { + "001", "2014-08-24 18:00:00", "3", "0", "21", "19", "0", "", "ROW_END", "CSV_END"}; + + // If the parser is stuck, the sentinel appears and this assertion helps diagnose. + ASSERT_EQ(tokens.back(), "CSV_END") << "Parser did not reach CSV_END; tokens: " << tokens.back(); + + // Compare full sequence. + EXPECT_EQ(tokens, expected); +} + +TEST(CsvParserBug, TrailingDelimiterCRLF) { + // Same scenario, but with Windows line ending to ensure CRLF handling works. + const std::string csv = "A|B|C|\r\n"; + + std::istringstream iss(csv); + CsvParser parser(iss); + parser.delimiter('|'); // default terminator is CRLF + + auto tokens = scanTokens(parser); + + std::vector expected = {"A", "B", "C", "", "ROW_END", "CSV_END"}; + + ASSERT_EQ(tokens.back(), "CSV_END") << "Parser did not reach CSV_END; tokens: " << tokens.back(); + EXPECT_EQ(tokens, expected); +} From d3ac028a71a9c446189a7225d7008c5d68d8419b Mon Sep 17 00:00:00 2001 From: azimafroozeh Date: Mon, 28 Jul 2025 16:17:10 +0200 Subject: [PATCH 2/3] Fix CsvParser infinite ROW_END loop for trailing empty fields --- data/include/data/issues.hpp | 15 ++++---- data/issues/000055/data.csv | 2 ++ data/issues/000055/schema.json | 44 +++++++++++++++++++++++ src/include/fls/csv/csv-parser/parser.hpp | 24 +++++++------ test/src/dataset_tests/issue.cpp | 5 --- test/src/dataset_tests/issues.cpp | 5 +++ 6 files changed, 72 insertions(+), 23 deletions(-) create mode 100644 data/issues/000055/data.csv create mode 100644 data/issues/000055/schema.json delete mode 100644 test/src/dataset_tests/issue.cpp diff --git a/data/include/data/issues.hpp b/data/include/data/issues.hpp index 1120a22a..bda4b2cd 100644 --- a/data/include/data/issues.hpp +++ b/data/include/data/issues.hpp @@ -10,7 +10,7 @@ #include namespace fastlanes { -using issues_dataset_t = std::array, 3>; +using issues_dataset_t = std::array, 4>; class issues { public: @@ -19,12 +19,13 @@ class issues { static constexpr std::string_view issues_cwida_alp_37_diff_data {FASTLANES_DATA_DIR "/issues/cwida/alp/37/diff_data"}; static constexpr std::string_view ISSUE_000 {FLS_CMAKE_SOURCE_DIR "/data/issues/issue_000/"}; - // - static constexpr issues_dataset_t dataset = {{ - {"issues_cwida_alp_37_kv_cache_original", issues_cwida_alp_37_kv_cache_original}, - {"issues_cwida_alp_37_diff_data", issues_cwida_alp_37_diff_data}, - {"ISSUE_000", ISSUE_000}, - }}; + static constexpr std::string_view ISSUE_055 {FLS_CMAKE_SOURCE_DIR "/data/issues/000055/"}; + + static constexpr issues_dataset_t dataset = { + {{"issues_cwida_alp_37_kv_cache_original", issues_cwida_alp_37_kv_cache_original}, + {"issues_cwida_alp_37_diff_data", issues_cwida_alp_37_diff_data}, + {"ISSUE_000", ISSUE_000}, + {ISSUE_055, ISSUE_055}}}; }; } // namespace fastlanes diff --git a/data/issues/000055/data.csv b/data/issues/000055/data.csv new file mode 100644 index 00000000..1161b43e --- /dev/null +++ b/data/issues/000055/data.csv @@ -0,0 +1,2 @@ +001|2014-09-24 18:00:00|12|2|24|17|0| +001|2014-08-24 18:00:00|3|0|21|19|0|0 diff --git a/data/issues/000055/schema.json b/data/issues/000055/schema.json new file mode 100644 index 00000000..7528c492 --- /dev/null +++ b/data/issues/000055/schema.json @@ -0,0 +1,44 @@ +{ + "columns": [ + { + "name": "Column_1", + "type": "BIGINT", + "index": "0" + }, + { + "name": "Column_2", + "type": "TIMESTAMP", + "index": "1" + }, + { + "name": "Column_3", + "type": "BIGINT", + "index": "2" + }, + { + "name": "Column_4", + "type": "BIGINT", + "index": "3" + }, + { + "name": "Column_5", + "type": "BIGINT", + "index": "4" + }, + { + "name": "Column_6", + "type": "BIGINT", + "index": "5" + }, + { + "name": "Column_7", + "type": "DOUBLE", + "index": "6" + }, + { + "name": "Column_7", + "type": "BIGINT", + "index": "6" + } + ] +} diff --git a/src/include/fls/csv/csv-parser/parser.hpp b/src/include/fls/csv/csv-parser/parser.hpp index d4441d36..8247973a 100644 --- a/src/include/fls/csv/csv-parser/parser.hpp +++ b/src/include/fls/csv/csv-parser/parser.hpp @@ -6,7 +6,8 @@ // https://github.com/AriaFallah/csv-parser // FLS_CHG -// ADD LAST_FIELD_EMPTY for cases like ||\n +// Handle empty last field at row end without a separate LAST_FIELD_EMPTY state, +// and ensure pending ROW_END is emitted before CSV_END at EOF. // // NOLINTBEGIN @@ -62,7 +63,6 @@ class CsvParser { // CSV state for state machine enum class State { START_OF_FIELD, // - LAST_FIELD_EMPTY, IN_FIELD, IN_QUOTED_FIELD, IN_ESCAPED_QUOTE, @@ -136,6 +136,12 @@ class CsvParser { // Reads a single field from the CSV Field next_field() { + // If we just finished a row previously, emit its ROW_END now + if (m_state == State::END_OF_ROW) { + m_state = State::START_OF_FIELD; + return Field(FieldType::ROW_END); + } + if (empty()) { return Field(FieldType::CSV_END); } @@ -158,16 +164,13 @@ class CsvParser { switch (m_state) { case State::START_OF_FIELD: m_cursor++; - // FLS_CHG - // if (c == m_terminator) { - // handle_crlf(c); - // return Field(FieldType::ROW_END); - // } // Handles multiline strings. Disabled by default, but we enable it as we have multiline strings. if (c == m_quote) { // && false) { // forget about quoting, our csv input is not legal anyway m_state = State::IN_QUOTED_FIELD; } else if (c == m_terminator) { - m_state = State::LAST_FIELD_EMPTY; + // Empty last field at end of row + handle_crlf(c); + m_state = State::END_OF_ROW; return Field(m_fieldbuf); } else if (c == m_delimiter) { return Field(m_fieldbuf); @@ -227,12 +230,11 @@ class CsvParser { break; case State::END_OF_ROW: + // Normally unreachable because we emit ROW_END early at the top, + // but keep the fallback for safety. m_state = State::START_OF_FIELD; return Field(FieldType::ROW_END); - case State::LAST_FIELD_EMPTY: - return Field(FieldType::ROW_END); - case State::EMPTY: throw std::logic_error("You goofed"); } diff --git a/test/src/dataset_tests/issue.cpp b/test/src/dataset_tests/issue.cpp deleted file mode 100644 index add9ef05..00000000 --- a/test/src/dataset_tests/issue.cpp +++ /dev/null @@ -1,5 +0,0 @@ -// ──────────────────────────────────────────────────────── -// | FastLanes | -// ──────────────────────────────────────────────────────── -// test/src/dataset_tests/issue.cpp -// ──────────────────────────────────────────────────────── diff --git a/test/src/dataset_tests/issues.cpp b/test/src/dataset_tests/issues.cpp index 304acd23..442f8416 100644 --- a/test/src/dataset_tests/issues.cpp +++ b/test/src/dataset_tests/issues.cpp @@ -20,4 +20,9 @@ TEST_F(FastLanesReaderTester, issue_000) { TestCorrectness(issues::ISSUE_000); } +TEST_F(FastLanesReaderTester, issue_055) { + TestCorrectness(issues::ISSUE_055); +} + + } // namespace fastlanes From afb9369fdf9a41a7b9f577ec9d507c71532ae664 Mon Sep 17 00:00:00 2001 From: azimafroozeh Date: Mon, 28 Jul 2025 16:18:57 +0200 Subject: [PATCH 3/3] fix-header bump --- test/src/dataset_tests/issues.cpp | 1 - test/src/quick_fuzz_tests/fuzz_config.json | 2 +- test/src/unit_tests/csv_parser_trailing_delimiter_test.cpp | 5 +++++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/test/src/dataset_tests/issues.cpp b/test/src/dataset_tests/issues.cpp index 442f8416..62286fe5 100644 --- a/test/src/dataset_tests/issues.cpp +++ b/test/src/dataset_tests/issues.cpp @@ -24,5 +24,4 @@ TEST_F(FastLanesReaderTester, issue_055) { TestCorrectness(issues::ISSUE_055); } - } // namespace fastlanes diff --git a/test/src/quick_fuzz_tests/fuzz_config.json b/test/src/quick_fuzz_tests/fuzz_config.json index 9d9e6df3..1f037baa 100644 --- a/test/src/quick_fuzz_tests/fuzz_config.json +++ b/test/src/quick_fuzz_tests/fuzz_config.json @@ -1,6 +1,6 @@ { "num_cases": 10, - "base_seed": 6, + "base_seed": 7, "delimiter": "|", "min_cols": 1, "max_cols": 2, diff --git a/test/src/unit_tests/csv_parser_trailing_delimiter_test.cpp b/test/src/unit_tests/csv_parser_trailing_delimiter_test.cpp index 48a8e62d..7b572041 100644 --- a/test/src/unit_tests/csv_parser_trailing_delimiter_test.cpp +++ b/test/src/unit_tests/csv_parser_trailing_delimiter_test.cpp @@ -1,3 +1,8 @@ +// ──────────────────────────────────────────────────────── +// | FastLanes | +// ──────────────────────────────────────────────────────── +// test/src/unit_tests/csv_parser_trailing_delimiter_test.cpp +// ──────────────────────────────────────────────────────── #include "fls/common/alias.hpp" #include "fls/common/assert.hpp" #include "fls/csv/csv-parser/parser.hpp"