Skip to content

Commit

Permalink
Fix filter crash (#5720)
Browse files Browse the repository at this point in the history 
* Fix filter crash

A bug happened where if you entered in new data:
(one by one)
column1, "c", "1"
column2, "2"

the engine would crash because of some bad housekeeping of data...

this is because labelsAdd was used which checks the valDisMap and then doesnt really add anything.
The db was iterating over rows though...
But ive now replaced it with column->labelsSet.
It takes the honor of either adding a label or overwriting one on a particular place.
And adds them to the also freshly cleaned valDisMap and intsIdMap.
Also, try not to resize the labels to be bigger then it needs to be to avoid all those 0pointer exceptions 😿

* if one wants to replace both the orignal value and display of a label do it atomically
  • Loading branch information
@JorisGoosen
JorisGoosen authored Oct 24, 2024
1 parent e975c43 commit c741721
Show file tree
Hide file tree
Showing 6 changed files with 134 additions and 43 deletions.
94 changes: 77 additions & 17 deletions CommonData/column.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ void Column::dbLoad(int id, bool getValues)

_emptyValues->fromJson(emptyVals);

labelsTempReset();
_resetLabelValueMap();
db().labelsLoad(this);

if(getValues)
Expand Down Expand Up @@ -721,6 +721,17 @@ int Column::labelsAdd(const std::string & display, const std::string & descripti
return labelsAdd(++_highestIntsId, display, true, description, originalValue);
}

int Column::_labelMapIt(Label * label)
{
_labelByIntsIdMap[label->intsId()] = label;
_labelByValDis[label->origValDisplay()] = label;

_highestIntsId = std::max(_highestIntsId, label->intsId());

_dbUpdateLabelOrder(true);
return label->intsId();
}

int Column::labelsAdd(int value, const std::string & display, bool filterAllows, const std::string & description, const Json::Value & originalValue, int order, int id)
{
JASPTIMER_SCOPE(Column::labelsAdd lotsa arg);
Expand All @@ -733,13 +744,36 @@ int Column::labelsAdd(int value, const std::string & display, bool filterAllows,
Label * label = new Label(this, display, value, filterAllows, description, originalValue, order, id);
_labels.push_back(label);

_labelByIntsIdMap[label->intsId()] = label;
_labelByValDis[valDisplay] = label;
return _labelMapIt(label);
}

_highestIntsId = std::max(_highestIntsId, label->intsId());
int Column::labelsSet(int labelIndex, int value, const std::string &display, bool filterAllows, const std::string &description, const Json::Value &originalValue, int order, int id)
{
JASPTIMER_SCOPE(Column::labelsSet);

_dbUpdateLabelOrder(true);
return label->intsId();
Label * label = nullptr;
if(_labels.size() > labelIndex)
{
label = _labels[labelIndex];
label->setInformation(this, id, order, display, value, filterAllows, description, originalValue);
}
else
{
if(_labels.size() != labelIndex)
Log::log() << "Labels are not being set in a sensible order it seems." << std::endl;

if(id == -1)
Log::log() << "This functions expects a label to be set from the DB, so why is dbId -1?" << std::endl;

if(_labels.size() < labelIndex+1)
_labels . resize(labelIndex+1);
_labels[labelIndex] = new Label(this, display, value, filterAllows, description, originalValue, order, id);
label = _labels[labelIndex];
}

assert(label);

return _labelMapIt(label);
}

void Column::labelsRemoveByIntsId(std::set<int> valuesToRemove, bool updateOrder)
Expand Down Expand Up @@ -809,7 +843,8 @@ void Column::labelsRemoveBeyond(size_t desiredLabelsSize)
for(size_t i=desiredLabelsSize; i<_labels.size(); i++)
delete _labels[i];

_labels.resize(desiredLabelsSize);
if(desiredLabelsSize < _labels.size())
_labels.resize(desiredLabelsSize);

_resetLabelValueMap();
}
Expand Down Expand Up @@ -1350,7 +1385,7 @@ bool Column::replaceDoubleLabelFromRowWithDouble(size_t row, double dbl)
return true;
}

void Column::labelValueChanged(Label *label, double aDouble, const Json::Value & previousOriginal)
void Column::labelValueChanged(Label *label, const Json::Value & previousOriginal)
{
auto oldValDis = std::make_pair(Label::originalValueAsString(this, previousOriginal), label->labelDisplay());
bool merged = _labelByValDis.count(label->origValDisplay()) != 0;
Expand All @@ -1363,17 +1398,11 @@ void Column::labelValueChanged(Label *label, double aDouble, const Json::Value &
//And that its new location is free:
assert(_labelByValDis.count(label->origValDisplay()) == 0 || _labelByValDis.at(label->origValDisplay()) == label);

//Lets assume that all occurences of a label in _dbls are the same.
//So when we encounter one that is the same as what is passed here we can return immediately
double theDouble = label->originalValue().isDouble() ? label->originalValue().asDouble() : EmptyValues::missingValueDouble;

for(size_t r=0; r<_dbls.size(); r++)
if(_ints[r] == label->intsId())
{
if(Utils::isEqual(_dbls[r], aDouble))
return;

_dbls[r] = aDouble;
}

_dbls[r] = theDouble;

_labelByValDis.erase(oldValDis);
_labelByValDis[label->origValDisplay()] = label;
Expand Down Expand Up @@ -1422,6 +1451,37 @@ void Column::labelDisplayChanged(Label *label, const std::string & previousDispl
_labelsTempRevision++;
}

void Column::labelValDisplayChanged(Label *label, const std::string &previousDisplay, const Json::Value &previousOriginal)
{
auto oldValDis = std::make_pair(Label::originalValueAsString(this, previousOriginal), previousDisplay),
newValDis = std::make_pair(label->originalValueAsString(), label->labelDisplay());
bool merged = _labelByValDis.count(label->origValDisplay()) != 0;

if(merged)
labelsMergeDuplicateInto(label);

//Make sure it was registered before:
assert(_labelByValDis[oldValDis] == label);
//And that its new location is free:
assert(_labelByValDis.count(label->origValDisplay()) == 0 || _labelByValDis.at(label->origValDisplay()) == label);

double newOrigValDbl = label->originalValue().isDouble() ? label->originalValue().asDouble() : EmptyValues::missingValueDouble;

//Lets assume that all occurences of a label in _dbls are the same.
//So when we encounter one that is the same as what is passed here we can return immediately
for(size_t r=0; r<_dbls.size(); r++)
if(_ints[r] == label->intsId())
_dbls[r] = newOrigValDbl;

_labelByValDis.erase(oldValDis);
_labelByValDis[label->origValDisplay()] = label;

if(merged)
_dbUpdateLabelOrder();

dbUpdateValues();
}

Label * Column::labelByRow(int row) const
{
if (row < rowCount() && _type != columnType::scale && _ints[row] != EmptyValues::missingValueInteger)
Expand Down
9 changes: 6 additions & 3 deletions CommonData/column.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ class Column : public DataSetBaseNode
int labelsAdd( const std::string & display);
int labelsAdd( const std::string & display, const std::string & description, const Json::Value & originalValue);
int labelsAdd( int value, const std::string & display, bool filterAllows, const std::string & description, const Json::Value & originalValue, int order=-1, int id=-1);
int labelsSet(int lbId, int value, const std::string & display, bool filterAllows, const std::string & description, const Json::Value & originalValue, int order=-1, int id=-1);
void labelsRemoveByIntsId( intset valuesToRemove, bool updateOrder = true);
strintmap labelsResetValues( int & maxValue);
void labelsRemoveBeyond( size_t indexToStartRemoving);
Expand Down Expand Up @@ -153,9 +154,9 @@ class Column : public DataSetBaseNode
Label * replaceDoublesTillLabelsRowWithLabels(size_t row, double returnForDbl = NAN);
bool replaceDoubleLabelFromRowWithDouble(size_t row, double dbl); ///< Returns true if succes

void labelValueChanged(Label * label, double aDouble, const Json::Value & previousOriginal); ///< Pass NaN for non-convertible values
void labelValueChanged(Label * label, int anInteger, const Json::Value & previousOriginal) { labelValueChanged(label, double(anInteger), previousOriginal); }
void labelDisplayChanged(Label * label, const std::string & previousDisplay);
void labelValueChanged( Label * label, const Json::Value & previousOriginal); ///< Pass NaN for non-convertible values
void labelDisplayChanged( Label * label, const std::string & previousDisplay);
void labelValDisplayChanged( Label * label, const std::string & previousDisplay, const Json::Value & previousOriginal);

bool setStringValue( size_t row, const std::string & value, const std::string & label = "", bool writeToDB = true); ///< Does two things, if label=="" it will handle user input, as value or label depending on columnType. Otherwise it will simply try to use userEntered as a value. But this will trigger the setting of type
bool setValue( size_t row, const std::string & value, const std::string & label, bool writeToDB = true);
Expand Down Expand Up @@ -244,6 +245,7 @@ class Column : public DataSetBaseNode
void _resetLabelValueMap();
doublevec valuesNumericOrdered();
std::map<Label*,size_t> valuesAlphabeticalOffsets();
int _labelMapIt(Label *label);

private:
DataSet * const _data;
Expand Down Expand Up @@ -282,6 +284,7 @@ class Column : public DataSetBaseNode
static bool _autoSortByValuesByDefault;



};

typedef std::vector<Column*> Columns;
Expand Down
6 changes: 2 additions & 4 deletions CommonData/databaseinterface.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1290,14 +1290,12 @@ void DatabaseInterface::labelsLoad(Column * column)

if (originalValueJson.isNull() && !originalValueJsonStr.empty())
originalValueJson = originalValueJsonStr; // For backward compatibility: in some JASP files the originalValueJson is not a json string but just the original string.

if(column->labels().size() == row) column->labelsAdd(value, label, filterAllows, description, originalValueJson, order, id);
else column->labels()[row]->setInformation(column, id, order, label, value, filterAllows, description, originalValueJson);

column->labelsSet(row, value, label, filterAllows, description, originalValueJson, order, id);
};

runStatements("SELECT id, value, label, ordering, filterAllows, description, originalValueJson FROM Labels WHERE columnId = ? ORDER BY ordering;", prepare, processRow);

column->labelsRemoveBeyond(labelsSize);

column->endBatchedLabelsDB(false);
Expand Down
41 changes: 33 additions & 8 deletions CommonData/label.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ void Label::dbUpdate()

void Label::setInformation(Column * column, int id, int order, const std::string &label, int value, bool filterAllows, const std::string & description, const Json::Value & originalValue)
{
_dbId = id;
assert(_column == column);
_dbId = id;
_order = order;
_label = label;
_intsId = value;
Expand Down Expand Up @@ -146,23 +147,47 @@ bool Label::setLabel(const std::string & label)
return false;
}

bool Label::setOriginalValue(const Json::Value & originalLabel)
bool Label::setOriginalValue(const Json::Value & originalValue)
{
if(_originalValue != originalLabel)
if(_originalValue != originalValue)
{
Json::Value previous = _originalValue;
_originalValue = originalLabel;
Json::Value previous = _originalValue;
_originalValue = originalValue;
dbUpdate();

if(_originalValue.isDouble()) _column->labelValueChanged(this, _originalValue.asDouble(), previous);
else if(_originalValue.isInt()) _column->labelValueChanged(this, _originalValue.asInt(), previous);
else _column->labelValueChanged(this, EmptyValues::missingValueDouble, previous);
_column->labelValueChanged(this, previous);

return true;
}
return false;
}

bool Label::setOrigValLabel(const Json::Value &originalValue)
{
std::string oldLabel = _label,
newLabel = !originalValue.isDouble() ? originalValue.asString() : _column->doubleToDisplayString(originalValue.asDouble(), false, false);
Json::Value previous = _originalValue;
bool labelChange = _label != newLabel,
valChange = _originalValue != originalValue,
aChange = labelChange || valChange;

if(labelChange)
_label = newLabel;

if(valChange)
_originalValue = originalValue;


if(aChange)
{
dbUpdate();

_column->labelValDisplayChanged(this, oldLabel, previous);
return true;
}
return false;
}

bool Label::setDescription(const std::string &description)
{
if(_description != description)
Expand Down
1 change: 1 addition & 0 deletions CommonData/label.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class Label : public DataSetBaseNode
void setDbId( int id) { _dbId = id; }
bool setLabel( const std::string & label);
bool setOriginalValue( const Json::Value & originalValue);
bool setOrigValLabel( const Json::Value & originalValue);
bool setDescription( const std::string & description);
bool setFilterAllows( bool allowFilter);
void setInformation(Column * column, int id, int order, const std::string &label, int value, bool filterAllows, const std::string & description, const Json::Value & originalValue);
Expand Down
26 changes: 15 additions & 11 deletions Desktop/data/datasetpackage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,7 @@ bool DataSetPackage::setData(const QModelIndex &index, const QVariant &value, in
setManualEdits(true); //Don't synch with external file after editing

column->labelsRemoveOrphans();
column->labelsTempReset();
column->labelsHandleAutoSort();

stringvec changedCols = {column->name()};
Expand Down Expand Up @@ -951,20 +952,23 @@ bool DataSetPackage::setLabelValue(const QModelIndex &index, const QString &newL
aChange = true;
}

//If the user is changing the value of a column to a string we want the display/label to also change if its the same
//to a double/int however might mean recoding so it would be a bit impractical to have the label dissappear
if( label->originalValueAsString(false) == label->labelDisplay())
{
if(!originalValue.isDouble())
aChange = label->setLabel(originalValue.asString()) || aChange;
// Here we will overwrite the original value with the new origval.
// but if the label is the same as the original value we want to make the users life easier and replace it as well.
// this makes sense if the user is changing a string or number. But if the user is recoding, so turning values from str => dbl
// then we dont want to do this, because then the label should be different afterwards.

else if(label->originalValue().isDouble())
label->setLabel(column->doubleToDisplayString(originalValue.asDouble(), false));
//summarized:
// if orgval == label then:
// if (oldorigval == dbl && newOrigVal == dbl) || (olorigval != dbl && newOrigVal != dbl) then replace both
// if neworigval == dbl and oldorigval != dbl then replace only value

if( label->originalValueAsString(false) != label->labelDisplay() || (originalValue.isDouble() && !label->originalValue().isDouble()))
aChange = label->setOriginalValue(originalValue) || aChange;
else
aChange = label->setOrigValLabel(originalValue) || aChange;
}

aChange = label->setOriginalValue(originalValue) || aChange;

column->labelsHandleAutoSort();

if(aChange)
Expand Down Expand Up @@ -2195,8 +2199,8 @@ bool DataSetPackage::removeRows(int row, int count, const QModelIndex & aparent)
{
changed.push_back(column->name());

if(row+count > column->rowCount())
Log::log() << "???" << std::endl;
//if(row+count > column->rowCount())
// Log::log() << "???" << std::endl;

for(int r=row+count; r>row; r--)
column->rowDelete(r-1);
Expand Down

0 comments on commit c741721

Please sign in to comment.