Copilot commented on code in PR #51118:
URL: https://github.com/apache/arrow/pull/51118#discussion_r3954737022
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -59,6 +61,14 @@ Status MismatchingColumns(const InvalidRow& row) {
inline bool IsControlChar(uint8_t c) { return c < ' '; }
+template <bool IgnoreExtraColumns>
+constexpr bool ShouldWrite(bool ignoring_extra_field) {
+ if constexpr (IgnoreExtraColumns) {
+ return !ignoring_extra_field;
+ }
+ return true;
+}
Review Comment:
`ShouldWrite<false>(bool)` doesn't use its parameter, which can trigger
`-Wunused-parameter` in some builds (often promoted to errors). Explicitly mark
the parameter unused in the `IgnoreExtraColumns == false` instantiation.
##########
cpp/src/arrow/csv/lexing_internal.h:
##########
@@ -35,6 +35,25 @@ class SpecializedOptions {
static constexpr bool escaping = Escaping;
};
+template <typename... CompiledBools, typename Fn, typename... Rest>
+ requires std::invocable<Fn, CompiledBools...,
+ std::conditional_t<true, std::true_type, Rest>...>
+decltype(auto) DispatchBool(Fn&& fn, Rest... rest) {
Review Comment:
`arrow/csv/lexing_internal.h` now uses `std::invocable`, `std::true_type` /
`std::false_type`, `std::conditional_t`, and `std::forward` in `DispatchBool`,
but the header doesn't include `<concepts>`, `<type_traits>`, or `<utility>`.
This breaks compilation for TUs like `chunker.cc` that include this header
without those includes.
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -452,6 +493,10 @@ class BlockParserImpl {
AbortLine:
// Not a full line except perhaps if in final block
if (is_final) {
+ if constexpr (IgnoreExtraColumns) {
+ // Handle an implicit trailing empty field after a delimiter.
+ ignoring_extra_field = IsExtraField();
+ }
goto LineEnd;
}
Review Comment:
In `ParseLine`, when `ParseFinal()` ends exactly on a delimiter (no trailing
newline), `LineEnd` will emit the implicit trailing empty field without having
called `StartField(false)`. That means the field’s `quoted` flag can
incorrectly inherit from the previous column (affecting null handling when
`quoted_strings_can_be_null = false`). Consider starting the implicit empty
field before jumping to `LineEnd`.
--
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]
