Skip to content

Commit 7e73421

Browse files
kevinwilfongmeta-codesync[bot]
authored andcommitted
fix: Initialize null children in SelectiveMapAsStructColumnReader getValues (#17720)
Summary: Pull Request resolved: #17720 `SelectiveMapAsStructColumnReader::getValues()` iterates over all children of the result RowVector and calls `mutableRawNulls()` on each to initialize null bits. However, when the result RowVector comes from `prepareResult()` in `SelectiveStructColumnReaderBase`, non-ROW children are intentionally left as nullptr (only ROW children are pre-allocated, per the design documented at `SelectiveStructColumnReader.cpp:35-39`). Calling `mutableRawNulls()` on a nullptr child causes a SIGSEGV. This manifests when delta UPDATE files with MAP-typed columns are read via the Metalake merge-on-read path with `readFlatmapAsStruct` enabled. The `DeltaUpdateReader::loadValues()` copies delta values across Nimble stripes, which triggers lazy loading of MAP-as-struct columns, hitting the null children in `getValues()`. This diff creates properly-typed empty vectors for any null children before the null-initialization loop, matching the contract that all children start as null (all bits set to `kNull`) before `makeCopyRanges` populates the ones with actual data. Reviewed By: Yuhta Differential Revision: D107328181 fbshipit-source-id: f36e57228a53d0f765af3411ea85073c353cd270
1 parent fd8f0cf commit 7e73421

2 files changed

Lines changed: 64 additions & 1 deletion

File tree

velox/dwio/common/SelectiveRepeatedColumnReader.cpp

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -597,7 +597,22 @@ void SelectiveMapAsStructColumnReader::getValues(
597597
BaseVector::prepareForReuse(*result, rows.size());
598598
auto* resultRow = result->get()->asChecked<RowVector>();
599599
setComplexNulls(rows, *result);
600-
for (auto& child : resultRow->children()) {
600+
for (column_index_t i = 0; i < resultRow->childrenSize(); ++i) {
601+
auto& child = resultRow->childAt(i);
602+
// prepareForReuse() above reuses or reallocates existing children, but it
603+
// skips children left as nullptr by prepareResult() in
604+
// SelectiveStructColumnReaderBase, which only pre-allocates ROW-typed
605+
// children -- the non-ROW value columns of a flat-map-as-struct result are
606+
// left null. Unlike a regular struct, this reader scatters into its
607+
// children with copyRanges() rather than recreating them per batch, so it
608+
// must ensure they exist. Create any missing child here (directly at the
609+
// final size); resize the rest, which prepareForReuse() shrank to 0.
610+
if (FOLLY_UNLIKELY(!child)) {
611+
child =
612+
BaseVector::create(resultRow->type()->childAt(i), rows.size(), pool_);
613+
} else {
614+
child->resize(rows.size());
615+
}
601616
bits::fillBits(child->mutableRawNulls(), 0, rows.size(), bits::kNull);
602617
}
603618
numValues_ = rows.size();

velox/dwio/dwrf/test/ReaderTest.cpp

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3076,6 +3076,54 @@ TEST_F(TestReader, mapAsStructFilterAfterRead) {
30763076
assertEqualVectors(expected, batch);
30773077
}
30783078

3079+
TEST_F(TestReader, mapAsStructNullChildrenWithReuse) {
3080+
// Reproduces the SIGSEGV fixed by initializing null children in
3081+
// SelectiveMapAsStructColumnReader::getValues. A filter on the
3082+
// flat-map-as-struct column routes the parent struct reader to call
3083+
// getValues() directly. The result is not singly referenced (a second
3084+
// reference is held, as a real downstream consumer would), so prepareResult()
3085+
// reallocates it via fillRowVectorChildren(), which leaves the
3086+
// flat-map-as-struct's non-ROW children as nullptr. getValues() must
3087+
// initialize those children rather than dereference them.
3088+
//
3089+
// This is identical to mapAsStructFilterAfterRead except for the held
3090+
// reference that forces the reallocation path.
3091+
auto row = makeRowVector({
3092+
makeMapVector<int32_t, int64_t>({{{1, 4}, {2, 5}}, {}, {{1, 6}, {3, 7}}}),
3093+
makeRowVector(
3094+
{makeConstant<int64_t>(0, 3)}, [](auto i) { return i == 0; }),
3095+
});
3096+
auto [writer, reader] =
3097+
createWriterReader({row}, pool(), dataIoStats_, metadataIoStats_);
3098+
auto outType =
3099+
ROW({"c0", "c1"}, {ROW({"3", "1"}, BIGINT()), ROW({"c0"}, BIGINT())});
3100+
auto spec = std::make_shared<common::ScanSpec>("<root>");
3101+
spec->addAllChildFields(*outType);
3102+
auto* c0Spec = spec->childByName("c0");
3103+
c0Spec->setFlatMapAsStruct(true);
3104+
c0Spec->setFilter(std::make_shared<common::IsNotNull>());
3105+
spec->childByName("c1")->setFilter(std::make_shared<common::IsNotNull>());
3106+
RowReaderOptions rowReaderOpts;
3107+
rowReaderOpts.setScanSpec(spec);
3108+
auto rowReader = reader->createRowReader(rowReaderOpts);
3109+
VectorPtr batch = BaseVector::create(outType, 0, pool());
3110+
// Hold a second reference so the result is not singly referenced; this forces
3111+
// prepareResult() down the fillRowVectorChildren() path, producing a
3112+
// flat-map-as-struct child whose non-ROW children are null.
3113+
VectorPtr holder = batch;
3114+
ASSERT_EQ(rowReader->next(10, batch), 3);
3115+
auto expected = makeRowVector({
3116+
makeRowVector(
3117+
{"3", "1"},
3118+
{
3119+
makeNullableFlatVector<int64_t>({std::nullopt, 7}),
3120+
makeNullableFlatVector<int64_t>({std::nullopt, 6}),
3121+
}),
3122+
makeRowVector({makeConstant<int64_t>(0, 2)}),
3123+
});
3124+
assertEqualVectors(expected, batch);
3125+
}
3126+
30793127
TEST_F(TestReader, mapAsStructAllEmpty) {
30803128
auto row = makeRowVector({makeMapVector<int32_t, int64_t>({{}, {}})});
30813129
auto [writer, reader] =

0 commit comments

Comments
 (0)