Skip to content

Commit

Permalink
[DAPHNE-#492] Creating Subframe of Frame with zero rows (#496)
Browse files Browse the repository at this point in the history
- Add test to check for crashes when creating sub-frame from Frame with zero rows.
- Add fix to enable creation of a sub-frame with zero rows from a Frame with zero rows, by excluding the assertions.
- Closes #492.
  • Loading branch information
tomschw authored Apr 11, 2023
1 parent fca5f15 commit 724e3cd
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 3 deletions.
10 changes: 7 additions & 3 deletions src/runtime/local/datastructures/Frame.h
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,13 @@ class Frame : public Structure {
Structure(rowUpperExcl - rowLowerIncl, numCols)
{
assert(src && "src must not be null");
assert((rowLowerIncl < src->numRows) && "rowLowerIncl is out of bounds");
assert((rowUpperExcl <= src->numRows) && "rowUpperExcl is out of bounds");
assert((rowLowerIncl < rowUpperExcl) && "rowLowerIncl must be lower than rowUpperExcl");

// Only check conditions, if input Frame has not zero rows and the expected output has not zero rows.
if(!(rowLowerIncl == rowUpperExcl && rowLowerIncl == 0 && src->numRows == 0)) {
assert((rowLowerIncl < src->numRows) && "rowLowerIncl is out of bounds");
assert((rowUpperExcl <= src->numRows) && "rowUpperExcl is out of bounds");
assert((rowLowerIncl < rowUpperExcl) && "rowLowerIncl must be lower than rowUpperExcl");
}
for(size_t i = 0; i < numCols; i++)
assert((colIdxs[i] < src->numCols) && "some colIdx is out of bounds");

Expand Down
37 changes: 37 additions & 0 deletions test/runtime/local/datastructures/FrameTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,41 @@ TEST_CASE("Frame column labels must be unique", TAG_DATASTRUCTURES) {
ValueTypeCode schema[] = {ValueTypeCode::SI64, ValueTypeCode::F64, ValueTypeCode::UI8};
const std::string labels[] = {"foo", "bar", "foo"};
CHECK_THROWS(DataObjectFactory::create<Frame>(4, 3, schema, labels, false));
}

TEST_CASE("Frame sub-frame for empty source frame works properly", TAG_DATASTRUCTURES) {
const size_t numRowsOrig = 0;
const ValueTypeCode schemaOrig[] = {ValueTypeCode::SI8, ValueTypeCode::UI32, ValueTypeCode::F64};
const size_t numColsOrig = sizeof(schemaOrig) / sizeof(ValueTypeCode);

Frame * fOrig = DataObjectFactory::create<Frame>(numRowsOrig, numColsOrig, schemaOrig, nullptr, true);
const size_t colIdxsSub[] = {2, 0};
const size_t numColsSub = sizeof(colIdxsSub) / sizeof(size_t);
Frame * fSub = DataObjectFactory::create<Frame>(fOrig, 0, 0, numColsSub, colIdxsSub);

// Sub-frame dimensions are as expected.
CHECK(fSub->getNumRows() == 0);
CHECK(fSub->getNumCols() == numColsSub);

// Sub-frame schema is as expected.
CHECK(fSub->getColumnType(0) == ValueTypeCode::F64);
CHECK(fSub->getColumnType(1) == ValueTypeCode::SI8);

// Sub-frame shares data arrays with original.
int8_t * colOrig0 = fOrig->getColumn<int8_t>(0)->getValues();
double * colOrig2 = fOrig->getColumn<double>(2)->getValues();
double * colSub0 = fSub->getColumn<double>(0)->getValues();
int8_t * colSub1 = fSub->getColumn<int8_t>(1)->getValues();
CHECK(colSub0 == colOrig2);
CHECK(colSub1 == colOrig0);

// Freeing both frames does not result in double-free errors.
SECTION("Freeing the original frame first is fine") {
DataObjectFactory::destroy(fOrig);
DataObjectFactory::destroy(fSub);
}
SECTION("Freeing the sub-frame first is fine") {
DataObjectFactory::destroy(fSub);
DataObjectFactory::destroy(fOrig);
}
}

0 comments on commit 724e3cd

Please sign in to comment.