Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow "" to filter for explicite empty fileds #1798

Merged
merged 15 commits into from
Dec 16, 2018
Merged

Allow "" to filter for explicite empty fileds #1798

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
28b0b67
Allow "" to filter for explicite empty fileds
daschuer Sep 2, 2018
76a1119
NullTextFilterNode::match matches also invalid values
daschuer Oct 4, 2018
b9e2be7
Fix typo
daschuer Oct 4, 2018
f54cf98
Use the first column esplicit
daschuer Oct 4, 2018
11ce5be
Use CATETABLE_ID
daschuer Oct 4, 2018
532c987
use a constant for ""
daschuer Oct 4, 2018
db56f9c
Improved query for tracks without crate
daschuer Oct 4, 2018
b817a32
Search for Null or Empty
daschuer Oct 8, 2018
dd1f85a
rename kExpliciteEmpty to kMissingFieldSearchTerm
daschuer Oct 8, 2018
defb3b8
Merge remote-tracking branch 'upstream/master' into 1788086
daschuer Oct 16, 2018
6811cf3
Fix building after merge
daschuer Oct 17, 2018
fc22212
use SELECT DISTINCT
daschuer Oct 21, 2018
5942d22
Added NullNumeric filter node
daschuer Oct 21, 2018
ef1d06a
Merge remote-tracking branch 'upstream/master' into 1788086
daschuer Dec 2, 2018
82cb592
added explicit keyword
daschuer Dec 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions src/library/crate/cratestorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@ CrateTrackQueryFields::CrateTrackQueryFields(const FwdSqlQuery& query)
m_iTrackId(query.fieldIndex(CRATETRACKSTABLE_TRACKID)) {
}

TrackQueryFields::TrackQueryFields(const FwdSqlQuery& query)
: m_iTrackId(query.fieldIndex(CRATETRACKSTABLE_TRACKID)) {
}

CrateSummaryQueryFields::CrateSummaryQueryFields(const FwdSqlQuery& query)
: CrateQueryFields(query),
Expand Down Expand Up @@ -462,6 +465,17 @@ QString CrateStorage::formatQueryForTrackIdsByCrateNameLike(
escapedCrateNameLike);
}

//static
QString CrateStorage::formatQueryForTrackIdsWithCrate() {
return QString("SELECT DISTINCT %1 FROM %2 JOIN %3 ON %4=%5 ORDER BY %1").arg(
CRATETRACKSTABLE_TRACKID,
CRATE_TRACKS_TABLE,
CRATE_TABLE,
CRATETRACKSTABLE_CRATEID,
CRATETABLE_ID);
}



CrateTrackSelectResult CrateStorage::selectCrateTracksSorted(CrateId crateId) const {
FwdSqlQuery query(m_database, QString(
Expand Down Expand Up @@ -514,8 +528,6 @@ CrateSummarySelectResult CrateStorage::selectCratesWithTrackCount(const QList<Tr
}
}



CrateTrackSelectResult CrateStorage::selectTracksSortedByCrateNameLike(const QString& crateNameLike) const {
FwdSqlQuery query(m_database, QString(
"SELECT %1,%2 FROM %3 JOIN %4 ON %5 = %6 WHERE %7 LIKE :crateNameLike ORDER BY %1").arg(
Expand All @@ -535,6 +547,17 @@ CrateTrackSelectResult CrateStorage::selectTracksSortedByCrateNameLike(const QSt
}
}

TrackSelectResult CrateStorage::selectAllTracksSorted() const {
FwdSqlQuery query(m_database, QString(
"SELECT DISTINCT %1 FROM %2 ORDER BY %1").arg(
CRATETRACKSTABLE_TRACKID, // %1
CRATE_TRACKS_TABLE)); // %2
if (query.execPrepared()) {
return TrackSelectResult(std::move(query));
} else {
return TrackSelectResult();
}
}

QSet<CrateId> CrateStorage::collectCrateIdsOfTracks(const QList<TrackId>& trackIds) const {
// NOTE(uklotzde): One query per track id. This could be optimized
Expand Down
39 changes: 39 additions & 0 deletions src/library/crate/cratestorage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,20 @@ class CrateTrackQueryFields {
DbFieldIndex m_iTrackId;
};

class TrackQueryFields {
public:
TrackQueryFields() = default;
explicit TrackQueryFields(const FwdSqlQuery& query);
virtual ~TrackQueryFields() = default;

TrackId trackId(const FwdSqlQuery& query) const {
return TrackId(query.fieldValue(m_iTrackId));
}

private:
DbFieldIndex m_iTrackId;
};

class CrateTrackSelectResult: public FwdSqlQuerySelectResult {
public:
CrateTrackSelectResult(CrateTrackSelectResult&& other)
Expand All @@ -176,6 +190,29 @@ class CrateTrackSelectResult: public FwdSqlQuerySelectResult {
CrateTrackQueryFields m_queryFields;
};

class TrackSelectResult: public FwdSqlQuerySelectResult {
public:
TrackSelectResult(TrackSelectResult&& other)
: FwdSqlQuerySelectResult(std::move(other)),
m_queryFields(std::move(other.m_queryFields)) {
}
~TrackSelectResult() override = default;

TrackId trackId() const {
return m_queryFields.trackId(query());
}

private:
friend class CrateStorage;
TrackSelectResult() = default;
explicit TrackSelectResult(FwdSqlQuery&& query)
: FwdSqlQuerySelectResult(std::move(query)),
m_queryFields(FwdSqlQuerySelectResult::query()) {
}

TrackQueryFields m_queryFields;
};

class CrateStorage: public virtual /*implements*/ SqlStorage {
public:
CrateStorage() = default;
Expand Down Expand Up @@ -268,6 +305,7 @@ class CrateStorage: public virtual /*implements*/ SqlStorage {

QString formatQueryForTrackIdsByCrateNameLike(
const QString& crateNameLike) const; // no db access
static QString formatQueryForTrackIdsWithCrate(); // no db access
// Select the track ids of a crate or the crate ids of a track respectively.
// The results are sorted (ascending) by the target id, i.e. the id that is
// not provided for filtering. This enables the caller to perform efficient
Expand All @@ -280,6 +318,7 @@ class CrateStorage: public virtual /*implements*/ SqlStorage {
const QList<TrackId>& trackIds) const;
CrateTrackSelectResult selectTracksSortedByCrateNameLike(
const QString& crateNameLike) const;
TrackSelectResult selectAllTracksSorted() const;

// Returns the set of crate ids for crates that contain any of the
// provided track ids.
Expand Down
90 changes: 90 additions & 0 deletions src/library/searchquery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
#include "library/queryutil.h"
#include "track/keyutils.h"
#include "library/dao/trackschema.h"
#include "library/crate/crateschema.h"
#include "util/db/sqllikewildcards.h"
#include "util/db/dbconnection.h"


QVariant getTrackValueForColumn(const TrackPointer& pTrack, const QString& column) {
if (column == LIBRARYTABLE_ARTIST) {
return pTrack->getArtist();
Expand Down Expand Up @@ -186,6 +188,26 @@ QString TextFilterNode::toSql() const {
return concatSqlClauses(searchClauses, "OR");
}

bool NullOrEmptyTextFilterNode::match(const TrackPointer& pTrack) const {
if (!m_sqlColumns.isEmpty()) {
// only use the major column
QVariant value = getTrackValueForColumn(pTrack, m_sqlColumns.first());
if (!value.isValid() || !value.canConvert(QMetaType::QString)) {
return true;
}
return value.toString().isEmpty();
}
return false;
}

QString NullOrEmptyTextFilterNode::toSql() const {
if (!m_sqlColumns.isEmpty()) {
// only use the major column
return QString("%1 IS NULL OR %1 IS ''").arg(m_sqlColumns.first());
}
return QString();
}

CrateFilterNode::CrateFilterNode(const CrateStorage* pCrateStorage,
const QString& crateNameLike)
: m_pCrateStorage(pCrateStorage),
Expand Down Expand Up @@ -213,9 +235,37 @@ QString CrateFilterNode::toSql() const {
m_pCrateStorage->formatQueryForTrackIdsByCrateNameLike(m_crateNameLike));
}


NoCrateFilterNode::NoCrateFilterNode(const CrateStorage* pCrateStorage)
: m_pCrateStorage(pCrateStorage),
m_matchInitialized(false) {
}

bool NoCrateFilterNode::match(const TrackPointer& pTrack) const {
if (!m_matchInitialized) {
TrackSelectResult tracks(
m_pCrateStorage->selectAllTracksSorted());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried, because generally the size of this query's result if of the same magnitude as the size of the whole library.

The only way for preventing this would be to use an adaptive strategy depending on the actual contents of the crate tracks table, i.e. for deciding if most tracks are organized in crates or not. This comes at the cost of one or two additional queries for determining the sizes upfront to chose the optimum query that will return either a blacklist or a whitelist.

But the whole search thing does not scale very well, so let's just keep it as is without introducing more complexity.


while (tracks.next()) {
m_matchingTrackIds.push_back(tracks.trackId());
}

m_matchInitialized = true;
}

return !std::binary_search(m_matchingTrackIds.begin(), m_matchingTrackIds.end(), pTrack->getId());
}

QString NoCrateFilterNode::toSql() const {
return QString("%1 NOT IN (%2)").arg(
CRATETABLE_ID,
CrateStorage::formatQueryForTrackIdsWithCrate());
}

NumericFilterNode::NumericFilterNode(const QStringList& sqlColumns)
: m_sqlColumns(sqlColumns),
m_bOperatorQuery(false),
m_bNullQuery(false),
m_operator("="),
m_dOperatorArgument(0.0),
m_bRangeQuery(false),
Expand All @@ -230,6 +280,11 @@ NumericFilterNode::NumericFilterNode(
}

void NumericFilterNode::init(QString argument) {
if (argument == kMissingFieldSearchTerm) {
m_bNullQuery = true;
return;
}

QRegExp operatorMatcher("^(>|>=|=|<|<=)(.*)$");
if (operatorMatcher.indexIn(argument) != -1) {
m_operator = operatorMatcher.cap(1);
Expand Down Expand Up @@ -264,6 +319,9 @@ bool NumericFilterNode::match(const TrackPointer& pTrack) const {
for (const auto& sqlColumn: m_sqlColumns) {
QVariant value = getTrackValueForColumn(pTrack, sqlColumn);
if (!value.isValid() || !value.canConvert(QMetaType::Double)) {
if (m_bNullQuery) {
return true;
}
continue;
}

Expand All @@ -285,6 +343,14 @@ bool NumericFilterNode::match(const TrackPointer& pTrack) const {
}

QString NumericFilterNode::toSql() const {
if (m_bNullQuery) {
for (const auto& sqlColumn: m_sqlColumns) {
// only use the major column
return QString("%1 IS NULL").arg(sqlColumn);
}
return QString();
}

if (m_bOperatorQuery) {
QStringList searchClauses;
for (const auto& sqlColumn: m_sqlColumns) {
Expand All @@ -310,6 +376,30 @@ QString NumericFilterNode::toSql() const {
return QString();
}

NullNumericFilterNode::NullNumericFilterNode(const QStringList& sqlColumns)
: m_sqlColumns(sqlColumns) {
}

bool NullNumericFilterNode::match(const TrackPointer& pTrack) const {
if (!m_sqlColumns.isEmpty()) {
// only use the major column
QVariant value = getTrackValueForColumn(pTrack, m_sqlColumns.first());
if (!value.isValid() || !value.canConvert(QMetaType::Double)) {
return true;
}
}
return false;
}

QString NullNumericFilterNode::toSql() const {
if (!m_sqlColumns.isEmpty()) {
// only use the major column
return QString("%1 IS NULL").arg(m_sqlColumns.first());
}
return QString();
}


DurationFilterNode::DurationFilterNode(
const QStringList& sqlColumns, const QString& argument)
: NumericFilterNode(sqlColumns) {
Expand Down
44 changes: 44 additions & 0 deletions src/library/searchquery.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#include "util/memory.h"
#include "library/crate/cratestorage.h"

const QString kMissingFieldSearchTerm = "\"\""; // "" searches for an empty string

QVariant getTrackValueForColumn(const TrackPointer& pTrack, const QString& column);

class QueryNode {
Expand Down Expand Up @@ -87,6 +89,23 @@ class TextFilterNode : public QueryNode {
QString m_argument;
};

class NullOrEmptyTextFilterNode : public QueryNode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have separate NullOrEmptyTextFilterNode/TextFilterNode and NoCrateFilterNode/FilterNode and the decision which one to use is done by the parser. But there is only one NumericFilterNode which does this decision internally.

I understand the motivation behind this, i.e. to separate and modularize the code for different use cases. But now the design has become inconsistent and responsibilities are scattered across different levels of abstraction.

public:
NullOrEmptyTextFilterNode(const QSqlDatabase& database,
const QStringList& sqlColumns)
: m_database(database),
m_sqlColumns(sqlColumns) {
}

bool match(const TrackPointer& pTrack) const override;
QString toSql() const override;

private:
QSqlDatabase m_database;
QStringList m_sqlColumns;
};


class CrateFilterNode : public QueryNode {
public:
CrateFilterNode(const CrateStorage* pCrateStorage,
Expand All @@ -102,6 +121,20 @@ class CrateFilterNode : public QueryNode {
mutable std::vector<TrackId> m_matchingTrackIds;
};

class NoCrateFilterNode : public QueryNode {
public:
NoCrateFilterNode(const CrateStorage* pCrateStorage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicit


bool match(const TrackPointer& pTrack) const override;
QString toSql() const override;

private:
const CrateStorage* m_pCrateStorage;
QString m_crateNameLike;
mutable bool m_matchInitialized;
mutable std::vector<TrackId> m_matchingTrackIds;
};

class NumericFilterNode : public QueryNode {
public:
NumericFilterNode(const QStringList& sqlColumns, const QString& argument);
Expand All @@ -124,13 +157,24 @@ class NumericFilterNode : public QueryNode {

QStringList m_sqlColumns;
bool m_bOperatorQuery;
bool m_bNullQuery;
QString m_operator;
double m_dOperatorArgument;
bool m_bRangeQuery;
double m_dRangeLow;
double m_dRangeHigh;
};

class NullNumericFilterNode : public QueryNode {
public:
NullNumericFilterNode(const QStringList& sqlColumns);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicit


bool match(const TrackPointer& pTrack) const override;
QString toSql() const override;

QStringList m_sqlColumns;
};

class DurationFilterNode : public NumericFilterNode {
public:
DurationFilterNode(const QStringList& sqlColumns, const QString& argument);
Expand Down
Loading