Fix the O(N^2) bug of passColName and passRowName

[metab0t]

Reland #1780

I also simplify some logic here. The result of emplace has contained the duplicate key-value pair if it detects duplication, so we can mark the value as duplicate directly instead of using the slow search-erase-insert combo.

[metab0t]

@jajhall CI tests pass. It is ready to be reviewed.

[jajhall]

By deleting

this->name2index.erase(search);
this->name2index.insert({name[index], kHashIsDuplicate});

and only setting

search->second = kHashIsDuplicate;

(which has no effect since search is immediately destroyed) the property of marking duplicate names is lost. hence errors due to the same name being given to different columns are not trapped.

[metab0t]

emplace will only insert when there is no duplication. When there is duplication, search points to the duplicated key and value, and setting the value will influence the original map.

This is an example: https://godbolt.org/z/YsYYrKrPb

[jajhall]

Thanks, it did cross my mind that there was more to setting

search->second = kHashIsDuplicate;

than I gave you credit for, sorry.

As I said, I'm not fluent in the use of std::unordered_map :-)

Just spotted the & in

auto& search = emplace_result.first;

Now I see why setting

search->second = kHashIsDuplicate;

achieves the desired result

[metab0t]

Never mind. I also learned about the meaning of return values of emplace today. It is a clever choice to use emplace to detect duplication.

[jajhall]

Ditto

[jajhall]

This may pass the CI tests, but that's because there's no unit test to check that duplicate names are marked as such

[metab0t]

This may pass the CI tests, but that's because there's no unit test to check that duplicate names are marked as such

HiGHS/check/TestNames.cpp

Lines 68 to 88 in 13363c9

	status = highs.getColByName(col0_name, iCol);
	REQUIRE(status == HighsStatus::kOk);
	REQUIRE(iCol == 0);
	status = highs.getRowByName(row0_name, iRow);
	REQUIRE(status == HighsStatus::kOk);
	REQUIRE(iRow == 0);
	// Change name of column num_col/2 to be the same as column 0
	REQUIRE(highs.getColName(0, name) == HighsStatus::kOk);
	REQUIRE(name == col0_name);
	iCol = lp.num_col_ / 2;
	std::string iCol_name;
	REQUIRE(highs.getColName(iCol, iCol_name) == HighsStatus::kOk);
	REQUIRE(highs.passColName(iCol, col0_name) == HighsStatus::kOk);
	// column num_col/2 is no longer called iCol_name
	status = highs.getColByName(iCol_name, iCol);
	REQUIRE(status == HighsStatus::kError);
	status = highs.getColByName(col0_name, iCol);
	REQUIRE(status == HighsStatus::kError);

In the beginning, col0_name is unique, so it has corresponding column.

But after col0_name is assigned to num_col_ / 2 column, it will be duplicate and cannot get column by the name.

[metab0t]

There is still one edge case: if a name is marked as duplicate and the corresponding column is assigned with a new name, then the old name should not be deleted directly (there might be other columns with this old name).

However, this is an edge case and checking it would require $O(N)$ iteration through the col_names_. In practice, I believe that few people will set the name of a variable twice.

The final and complete solution is to use https://en.cppreference.com/w/cpp/container/unordered_multimap to map name to column/row if we want to allow multiple columns/rows to have the same name.

[jajhall]

Indeed, there's a limit to making this idiot-proof