Alb3e3 commented on PR #50927:
URL: https://github.com/apache/arrow/pull/50927#issuecomment-5411768737
Thanks @pitrou — all four addressed in \`4c4ecb9390\`, replies inline.
Summary:
| Ask | Done |
| --- | --- |
| Move the two classes to \`arrow/testing/extension_type.h\` | Yes —
declarations there, bodies in \`testing/gtest_util.cc\` |
| Move the two functions to \`arrow/ipc/test_common.h\` | Yes — as
\`MakeDenseUnionExtension\` / \`MakeSparseUnionExtension\` |
| Extend \`kBatchCases\` instead of dedicated tests | Yes — the four
\`TEST_F\` blocks are gone |
| \`writer.cc\` \`arr\` vs \`physical_arr\` consistency | Yes — unwrapped
once into a reference, \`arr\` unused after |
Net effect on \`read_write_test.cc\` is -87/+20: the file now only registers
the two types in \`ExtensionTypesMixin\` and lists the two makers in
\`kBatchCases\`.
**Verification** — \`arrow-ipc-read-write-test\`: 431/432, the one failure
being \`TestSchemaMetadata.MetadataVersionForwardCompatibility\`, which is my
checkout missing the \`testing/\` submodule (\`Test resources not found, set
ARROW_TEST_DATA\`), not related to this change. \`arrow-extension-type-test\`
6/6 and \`arrow-feather-test\` 83/83 also pass, since both link
\`arrow_testing\`.
Non-vacuousness: reverting \`writer.cc\` and keeping the tests reproduces
the bug in \`TestFileFormat.RoundTrip/{27,28}\`,
\`TestIpcRoundTrip.RoundTrip/{27,28}\`,
\`TestIpcRoundTrip.ZeroLengthArrays/{27,28}\`, and aborts
\`TestFileFormatGenerator\` on `std::vector` bounds-checking of the ArrayData
children.
On the red CI: \`C++ Format\` was mine — \`read_write_test.cc\` was not
clang-format-18.1.8 clean at the previous head. That code is deleted now, and
all six touched files pass \`clang-format --dry-run -Werror\`. The other three
failures look unrelated to me — \`Python (Cython) Lint\` dies on
\`AttributeError: 'DictComprehensionAppendNode' object has no attribute
'value_expr'\`, and \`AMD64 Conda C++ AVX2\` fails only
\`arrow-filesystem-test\` with \`Attempt to initialize S3 after it has been
finalized\`. Happy to look if you think either is on me.
For transparency, and since it came up on another PR I have open: this work
is AI-assisted. The evidence above is all locally reproducible and I would
rather you have that context up front.
--
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]
