few Row sorting fixes/optimizations

Data/include/Poco/Data/Row.h

@@ -85,15 +85,31 @@ public:
 
 	enum ComparisonType
 	{
+		COMPARE_AS_EMPTY,
 		COMPARE_AS_INTEGER,
 		COMPARE_AS_FLOAT,
 		COMPARE_AS_STRING
 	};
 
+	typedef Tuple<std::size_t, ComparisonType> SortTuple;
+	typedef std::vector<SortTuple>             SortMap;
+		/// The type for map holding fields used for sorting criteria.
+		/// Fields are added sequentially and have precedence that
+		/// corresponds to field adding sequence order (rather than field's 
+		/// position in the row).
+		/// This requirement rules out use of std::map due to its sorted nature.
+	typedef SharedPtr<SortMap> SortMapPtr;
+
 	Row();
 		/// Creates the Row.
 
-	Row(NameVecPtr pNames, const RowFormatterPtr& pFormatter = 0);
+	Row(NameVecPtr pNames,
+		const RowFormatterPtr& pFormatter = 0);
+		/// Creates the Row.
+
+	Row(NameVecPtr pNames,
+		const SortMapPtr& pSortMap,
+		const RowFormatterPtr& pFormatter = 0);
 		/// Creates the Row.
 
 	~Row();
@@ -113,8 +129,7 @@ public:
 		/// Appends the value to the row.
 	{
 		if (!_pNames) _pNames = new NameVec;
-		DynamicAny da = val;
+		_values.push_back(val);
-		_values.push_back(da);
 		_pNames->push_back(name);
 		if (1 == _values.size()) addSortField(0);
 	}
@@ -200,20 +215,26 @@ public:
 	const ValueVec& values() const;
 		/// Returns the const reference to values vector.
 
-	void setFormatter(const RowFormatterPtr& pFormatter);
+	void setFormatter(const RowFormatterPtr& pFormatter = 0);
 		/// Sets the formatter for this row and takes the
 		/// shared ownership of it.
 
 	const RowFormatter& getFormatter() const;
 		/// Returns the reference to the formatter.
 
+	void setSortMap(const SortMapPtr& pSortMap = 0);
+		/// Adds the sorting fields entry and takes the
+		/// shared ownership of it.
+
+	const SortMapPtr& getSortMap() const;
+		/// Returns the reference to the sorting fields.
+
 private:
-	typedef Tuple<std::size_t, ComparisonType> SortTuple;
+	void init(const SortMapPtr& pSortMap, const RowFormatterPtr& pFormatter);
-	typedef std::vector<SortTuple> SortMap;
+
-		/// The type for map holding fields used for sorting criteria.
+	void checkEmpty(std::size_t pos, const DynamicAny& val);
-		/// Fields are added sequentially and have precedence that
+		/// Check if row contains only empty values and throws IllegalStateException
-		/// corresponds to adding order rather than field's position in the row.
+		/// if that is the case.
-		/// That requirement rules out use of std::map due to its sorted nature.
 
 	ValueVec& values();
 		/// Returns the reference to values vector.
@@ -224,7 +245,7 @@ private:
 
 	NameVecPtr           _pNames;
 	ValueVec             _values;
-	SortMap              _sortFields;
+	SortMapPtr           _pSortMap;
 	RowFormatterPtr      _pFormatter;
 	mutable std::string  _nameStr;
 	mutable std::string  _valueStr;
@@ -286,6 +307,12 @@ inline const RowFormatter& Row::getFormatter() const
 }
 
 
+inline const Row::SortMapPtr& Row::getSortMap() const
+{
+	return _pSortMap;
+}
+
+
 inline const std::string& Row::valuesToString() const
 {
 	return _pFormatter->formatValues(values(), _valueStr);

Data/src/RecordSet.cpp

@@ -168,9 +168,13 @@ Row& RecordSet::row(std::size_t pos)
 	std::size_t columns = columnCount();
 	if (it == _rowMap.end())
 	{
-		if (_rowMap.size())//reuse first row column names to save some memory 
+		if (_rowMap.size())
 		{
-			pRow = new Row(_rowMap.begin()->second->names(), getRowFormatter());
+			//reuse first row column names and sorting fields to save some memory 
+			pRow = new Row(_rowMap.begin()->second->names(),
+				_rowMap.begin()->second->getSortMap(),
+				getRowFormatter());
+
 			for (std::size_t col = 0; col < columns; ++col)
 				pRow->set(col, value(col, pos));
 		}

Data/src/Row.cpp

@@ -52,26 +52,43 @@ std::ostream& operator << (std::ostream &os, const Row& row)
 
 Row::Row(): 
 	_pNames(0),
+	_pSortMap(new SortMap),
 	_pFormatter(new SimpleRowFormatter)
 {
 }
 
 
-Row::Row(NameVecPtr pNames, const RowFormatterPtr& pFormatter): 
+Row::Row(NameVecPtr pNames,
-	_pNames(pNames)
+	const RowFormatterPtr& pFormatter): _pNames(pNames)
 {
 	if (!_pNames) throw NullPointerException();
+	init(0, pFormatter);
+}
 
+
+Row::Row(NameVecPtr pNames,
+	const SortMapPtr& pSortMap,
+	const RowFormatterPtr& pFormatter): _pNames(pNames)
+{
+	if (!_pNames) throw NullPointerException();
+	init(pSortMap, pFormatter);
+}
+
+
+void Row::init(const SortMapPtr& pSortMap, const RowFormatterPtr& pFormatter)
+{
 	setFormatter(pFormatter);
+	setSortMap(pSortMap);
 
 	NameVec::size_type sz = _pNames->size();
 	if (sz)
 	{
 		_values.resize(sz);
-		// Row sortability at all times is an invariant, hence 
+		// Row sortability in the strict weak ordering sense is 
-		// we must start with a zero here. If null value is later
+		// an invariant, hence we must start with a zero here.
-		// retrieved from DB, the DynamicAny::empty() call
+		// If null value is later retrieved from DB, the 
-		// should be used to empty the corresponding Row value.
+		// DynamicAny::empty() call should be used to empty
+		// the corresponding Row value.
 		_values[0] = 0;
 		addSortField(0);
 	}
@@ -113,22 +130,39 @@ std::size_t Row::getPosition(const std::string& name)
 }
 
 
+void Row::checkEmpty(std::size_t pos, const DynamicAny& val)
+{
+	bool empty = true;
+	SortMap::const_iterator it = _pSortMap->begin();
+	SortMap::const_iterator end = _pSortMap->end();
+	for (std::size_t cnt = 0; it != end; ++it, ++cnt)
+	{
+		if (cnt != pos)
+			empty = empty && _values[it->get<0>()].isEmpty();
+	}
+
+	if (empty && val.isEmpty())
+		throw IllegalStateException("All values are empty.");
+}
+
+
 void Row::addSortField(std::size_t pos)
 {
 	poco_assert (pos <= _values.size());
 
-	SortMap::iterator it = _sortFields.begin();
+	checkEmpty(std::numeric_limits<std::size_t>::max(), _values[pos]);
-	SortMap::iterator end = _sortFields.end();
+
+	SortMap::iterator it = _pSortMap->begin();
+	SortMap::iterator end = _pSortMap->end();
 	for (; it != end; ++it)
 	{
-		if (it->get<0>() == pos)
+		if (it->get<0>() == pos) return; 
-			throw InvalidAccessException("Field already in comparison set.");
 	}
 
 	ComparisonType ct;
 	if (_values[pos].isEmpty())
 	{
-		throw InvalidAccessException("Empty value not sortable.");
+		ct = COMPARE_AS_EMPTY;
 	}
 	else if ((_values[pos].type() == typeid(Poco::Int8))   ||
 		(_values[pos].type() == typeid(Poco::UInt8))  ||
@@ -152,7 +186,7 @@ void Row::addSortField(std::size_t pos)
 		ct = COMPARE_AS_STRING;
 	}
 
-	_sortFields.push_back(SortTuple(pos, ct));
+	_pSortMap->push_back(SortTuple(pos, ct));
 }
 
 
@@ -164,13 +198,15 @@ void Row::addSortField(const std::string& name)
 
 void Row::removeSortField(std::size_t pos)
 {
-	SortMap::iterator it = _sortFields.begin();
+	checkEmpty(pos, DynamicAny());
-	SortMap::iterator end = _sortFields.end();
+
+	SortMap::iterator it = _pSortMap->begin();
+	SortMap::iterator end = _pSortMap->end();
 	for (; it != end; ++it)
 	{
 		if (it->get<0>() == pos)
 		{
-			_sortFields.erase(it);
+			_pSortMap->erase(it);
 			return;
 		}
 	}
@@ -192,7 +228,7 @@ void Row::replaceSortField(std::size_t oldPos, std::size_t newPos)
 
 	if (_values[newPos].isEmpty())
 	{
-		throw InvalidAccessException("Empty value not sortable.");
+		ct = COMPARE_AS_EMPTY;
 	}
 	else if ((_values[newPos].type() == typeid(Poco::Int8)) ||
 		(_values[newPos].type() == typeid(Poco::UInt8))     ||
@@ -216,8 +252,8 @@ void Row::replaceSortField(std::size_t oldPos, std::size_t newPos)
 		ct = COMPARE_AS_STRING;
 	}
 
-	SortMap::iterator it = _sortFields.begin();
+	SortMap::iterator it = _pSortMap->begin();
-	SortMap::iterator end = _sortFields.end();
+	SortMap::iterator end = _pSortMap->end();
 	for (; it != end; ++it)
 	{
 		if (it->get<0>() == oldPos)
@@ -239,7 +275,7 @@ void Row::replaceSortField(const std::string& oldName, const std::string& newNam
 
 void Row::resetSort()
 {
-	_sortFields.clear();
+	_pSortMap->clear();
 	if (_values.size())	addSortField(0);
 }
 
@@ -289,15 +325,18 @@ bool Row::operator != (const Row& other) const
 
 bool Row::operator < (const Row& other) const
 {
-	if (_sortFields != other._sortFields)
+	if (*_pSortMap != *other._pSortMap)
 		throw InvalidAccessException("Rows compared have different sorting criteria.");
 
-	SortMap::const_iterator it = _sortFields.begin();
+	SortMap::const_iterator it = _pSortMap->begin();
-	SortMap::const_iterator end = _sortFields.end();
+	SortMap::const_iterator end = _pSortMap->end();
 	for (; it != end; ++it)
 	{
 		switch (it->get<1>())
 		{
+		case COMPARE_AS_EMPTY:
+			return false;
+
 		case COMPARE_AS_INTEGER:
 			if (_values[it->get<0>()].convert<Poco::Int64>() < 
 				other._values[it->get<0>()].convert<Poco::Int64>())
@@ -311,7 +350,7 @@ bool Row::operator < (const Row& other) const
 			if (_values[it->get<0>()].convert<double>() < 
 				other._values[it->get<0>()].convert<double>())
 				return true;
-			else if (_values[it->get<0>()].convert<double>() < 
+			else if (_values[it->get<0>()].convert<double>() !=
 				other._values[it->get<0>()].convert<double>())
 				return false;
 			break;
@@ -320,10 +359,13 @@ bool Row::operator < (const Row& other) const
 			if (_values[it->get<0>()].convert<std::string>() < 
 				other._values[it->get<0>()].convert<std::string>())
 				return true;
-			else if (_values[it->get<0>()].convert<std::string>() < 
+			else if (_values[it->get<0>()].convert<std::string>() !=
 				other._values[it->get<0>()].convert<std::string>())
 				return false;
 			break;
+
+		default:
+			throw IllegalStateException("Unknown comparison criteria.");
 		}
 	}
 
@@ -333,13 +375,22 @@ bool Row::operator < (const Row& other) const
 
 void Row::setFormatter(const RowFormatterPtr& pFormatter)
 {
-	if (pFormatter)
+	if (pFormatter.get())
 		_pFormatter = pFormatter;
 	else 
 		_pFormatter = new SimpleRowFormatter;
 }
 
 
+void Row::setSortMap(const SortMapPtr& pSortMap)
+{
+	if (pSortMap.get())
+		_pSortMap = pSortMap;
+	else 
+		_pSortMap = new SortMap;
+}
+
+
 const std::string& Row::namesToString() const
 {
 	if (!_pNames)

Data/testsuite/src/DataTest.cpp

@@ -47,6 +47,7 @@
 #include "Poco/BinaryWriter.h"
 #include "Poco/DateTime.h"
 #include "Poco/Types.h"
+#include "Poco/DynamicAny.h"
 #include "Poco/Exception.h"
 #include <cstring>
 #include <sstream>
@@ -63,7 +64,9 @@ using Poco::UInt32;
 using Poco::Int64;
 using Poco::UInt64;
 using Poco::DateTime;
+using Poco::DynamicAny;
 using Poco::InvalidAccessException;
+using Poco::IllegalStateException;
 using Poco::RangeException;
 using Poco::NotFoundException;
 using Poco::InvalidArgumentException;
@@ -876,10 +879,15 @@ void DataTest::testRow()
 
 	try
 	{
+		row4.set("field1", DynamicAny());
 		row4.addSortField(1);
+		row4.removeSortField(0);
 		fail ("must fail - field 1 is empty");
 	}
-	catch (InvalidAccessException&) { }
+	catch (IllegalStateException&)
+	{
+		row4.removeSortField(1);
+	}
 
 	row4.set("field0", 0);
 	row4.set("field1", 1);
@@ -1018,6 +1026,59 @@ void DataTest::testRowSort()
 	assert (row6 == *it3);
 	++it3;
 	assert (row5 == *it3);
+
+	Row row8;
+	row8.append("0", "2");
+	row8.append("1", "2");
+	row8.append("2", "0");
+	row8.append("3", "3");
+	row8.append("4", "4");
+	row8.addSortField("1");
+
+	Row row9;
+	row9.append("0", "1");
+	row9.append("1", "0");
+	row9.append("2", "1");
+	row9.append("3", "3");
+	row9.append("4", "4");
+	row9.addSortField("1");
+
+	Row row10;
+	row10.append("0", "0");
+	row10.append("1", "1");
+	row10.append("2", "2");
+	row10.append("3", "3");
+	row10.append("4", "4");
+	row10.addSortField("1");
+
+	testRowStrictWeak(row10, row9, row8);
+
+
+	Row row11;
+	row11.append("0", 2.5);
+	row11.append("1", 2.5);
+	row11.append("2", 0.5);
+	row11.append("3", 3.5);
+	row11.append("4", 4.5);
+	row11.addSortField("1");
+
+	Row row12;
+	row12.append("0", 1.5);
+	row12.append("1", 0.5);
+	row12.append("2", 1.5);
+	row12.append("3", 3.5);
+	row12.append("4", 4.5);
+	row12.addSortField("1");
+
+	Row row13;
+	row13.append("0", 0.5);
+	row13.append("1", 1.5);
+	row13.append("2", 2.5);
+	row13.append("3", 3.5);
+	row13.append("4", 4.5);
+	row13.addSortField("1");
+
+	testRowStrictWeak(row13, row12, row11);
 }