Allow "" to filter for explicite empty fileds #1798

daschuer · 2018-09-02T21:38:31Z

This allows to filter all Tracks without a genre like that genre:""
It works for all text fields, including crate:""

https://bugs.launchpad.net/mixxx/+bug/1788086

daschuer · 2018-09-11T19:40:04Z

The Appveyor failure is unrelated to this PR.

benis · 2018-10-01T17:52:06Z

I've tested this on Mac, where it appears to work as intended with no unintended side effects.

daschuer · 2018-10-02T05:49:43Z

Thank you for testing.
Do you also have interest to review the code, to get this finally merged?

benis · 2018-10-02T11:39:44Z

I would, but I don't think I know C++ or the syntax well enough for it to be a meaningful review.

uklotzde · 2018-10-03T11:32:30Z

src/library/searchquery.cpp

+bool NullTextFilterNode::match(const TrackPointer& pTrack) const {
+    for (const auto& sqlColumn: m_sqlColumns) {
+        QVariant value = getTrackValueForColumn(pTrack, sqlColumn);
+        if (!value.isValid() || !qVariantCanConvert<QString>(value)) {


This condition might cause that different columns are picked by match() and toSql(). toSql() simply picks the first column.

OK. This is only relevant for "artist" which has two columns. So I think in our case if the "artist" column is empty or invalid it should be enough to match.

uklotzde · 2018-10-03T11:32:37Z

src/library/searchquery.cpp

+        if (!value.isValid() || !qVariantCanConvert<QString>(value)) {
+            continue;
+        }
+        // only use the major coloumn


uklotzde · 2018-10-03T11:32:45Z

src/library/searchquery.cpp

+
+QString NullTextFilterNode::toSql() const {
+    for (const auto& sqlColumn: m_sqlColumns) {
+        // only use the major coloumn


uklotzde · 2018-10-03T11:33:26Z

src/library/searchquery.cpp

+}
+
+QString NullTextFilterNode::toSql() const {
+    for (const auto& sqlColumn: m_sqlColumns) {


This loop is confusing if you just want to pick the first column from a non-empty list.

uklotzde · 2018-10-03T11:36:26Z

src/library/searchquery.cpp

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


CRATETABLE_ID

uklotzde · 2018-10-03T11:39:52Z

src/library/searchqueryparser.cpp

@@ -153,8 +170,13 @@ void SearchQueryParser::parseTokens(QStringList tokens,
                    mixxx::track::io::key::ChromaticKey key =
                            KeyUtils::guessKeyFromText(argument);
                    if (key == mixxx::track::io::key::INVALID) {
-                        pNode = std::make_unique<TextFilterNode>(
-                                m_pTrackCollection->database(), m_fieldToSqlColumns[field], argument);
+                        if (argument == "\"\"") {


"""" should become a constant

uklotzde · 2018-10-03T11:47:09Z

src/library/searchquery.cpp

+             m_pCrateStorage->selectAllTracksSorted());
+
+        while (crateTracks.next()) {
+            m_matchingTrackIds.push_back(crateTracks.trackId());


Track IDs might be pushed multiple times, which is not necessary. The result contains tuples of (track id, crate id) sorted by track id, but we only need distinct track ids here.

We can also think about adding a new query instead of post-filtering the result.

uklotzde · 2018-10-03T11:52:47Z

Using "" for empty/missing numeric fields feels strange t first, but at least it is consistent. It might even be more intuitive for the common user, who does not consider the restrictions of a strongly typed programming language.

daschuer · 2018-10-04T21:59:52Z

Done. Thank you for review.

uklotzde · 2018-10-07T20:49:46Z

src/library/searchquery.h

@@ -16,6 +16,8 @@
 #include "util/memory.h"
 #include "library/crate/cratestorage.h"

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


The name of the constant is not a valid English term and needs to be explained. The comment is misleading, because it applies to all kind of fields and not only strings.

Maybe kMissingFieldSearchTerm or something else?

The SQL uses "IS NULL" which does not match fields with empty strings!? What is the desired result for string fields? I suggest to cover both NULL and empty strings with this filter.

uklotzde · 2018-10-07T20:50:55Z

src/library/searchquery.h

@@ -91,6 +93,23 @@ class TextFilterNode : public QueryNode {
    QString m_argument;
 };

+class NullTextFilterNode : public QueryNode {


The naming is inappropriate if we want to filter for both NULL and empty strings.

uklotzde · 2018-10-07T21:26:03Z

As expected empty string fields are not covered by this filter yet.

daschuer · 2018-10-08T12:10:59Z

Done!

uklotzde · 2018-10-08T22:11:15Z

After a closer look at the parser I really have to asked a question: Why don't we evaluate empty arguments like they are supposed to be?? The odd special case handling at the beginning of SearchQueryParser::getTextArgument() prevents us from using empty arguments for this use case. Instead we now add another special case by introducing this counterintuitive empty quoted argument workaround.

We should instead remove the existing special case and use empty arguments to search for missing or empty fields. Did you notice that only spaces inserted on the right side get this special case handling, but not those on the left side? One more reason for removing this ugly legacy code, even if it changes the behaviour. Only the regex should be responsible for tokenizing the filter strings into field and argument.

daschuer · 2018-10-16T21:48:56Z

I think there can be a way to keep some of the special case handling and allow to use an empty argument for searching for empty strings. As a last resort, we can also ditch the feature to allow a space after the token.

I think we need to define the big goal and adjust our small steps towards this. I also like to stick with the quoted search as an "exact match" like Google does. After the discussion how to find BPM values, I am convinced that the fuzzy match should be the default and if one wants to have the exact match, he has to use quotes.
This PR is already in line with this.

By the way a singles space after the token like "title: " may also search for all titles that contain a space aka two words.

What do you think?

Conflicts: src/library/searchquery.cpp

uklotzde · 2018-10-17T07:21:05Z

The idea for distinguishing between an exact and a fuzzy search not reflected in the code. Now it's just one more special case that makes refactoring the code harder when implementing the full-featured solution. That's my only (minor) concern. But the code has to be rewritten anyway.

Usability (also a minor issue): The feature is difficult to discover without reading the manual.

It's ok for this small change. But I don't like this way of piling up new features on top without cleaning up the mess below, even if this requires dropping old, inconsistent features.

daschuer · 2018-10-17T16:17:40Z

Ok, so how to continue here?
Merge it as it is or should I add the blankspace = emptyOrNull case first?

The first candidate for exact and fuzy match can be BPM.
"120" finds only 120.00 tracks wile plain 120 should match 119-121

uklotzde · 2018-10-19T11:55:30Z

src/library/crate/cratestorage.cpp

@@ -535,6 +547,17 @@ CrateTrackSelectResult CrateStorage::selectTracksSortedByCrateNameLike(const QSt
    }
 }

+TrackSelectResult CrateStorage::selectAllTracksSorted() const {
+    FwdSqlQuery query(m_database, QString(
+            "SELECT %1 FROM %2 GROUP BY %1 ORDER BY %1").arg(


Why do you use GROUP BY instead of DISTINCT?

uklotzde · 2018-10-19T12:10:38Z

src/library/searchquery.cpp

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


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.

uklotzde · 2018-10-19T12:20:40Z

src/library/searchquery.h

@@ -91,6 +93,23 @@ class TextFilterNode : public QueryNode {
    QString m_argument;
 };

+class NullOrEmptyTextFilterNode : public QueryNode {


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.

uklotzde · 2018-10-19T12:21:56Z

We can keep the functionality as is. But we have to at least fix the design issues to keep this maintainable.

daschuer · 2018-10-21T22:14:31Z

OK, done.
I have also considered to add new regular expressions, but I think it is much more performant to exam the found matches individually.

Conflicts: src/library/searchqueryparser.cpp

daschuer · 2018-12-15T23:49:19Z

@uklotzde, All comments are addressed, Can we merge this?

uklotzde · 2018-12-16T10:13:33Z

src/library/searchquery.h

    QString m_operator;
    double m_dOperatorArgument;
    bool m_bRangeQuery;
    double m_dRangeLow;
    double m_dRangeHigh;
 };

+class NullNumericFilterNode : public QueryNode {
+  public:
+    NullNumericFilterNode(const QStringList& sqlColumns);


uklotzde · 2018-12-16T10:13:40Z

src/library/searchquery.h

@@ -102,6 +121,20 @@ class CrateFilterNode : public QueryNode {
    mutable std::vector<TrackId> m_matchingTrackIds;
 };

+class NoCrateFilterNode : public QueryNode {
+  public:
+    NoCrateFilterNode(const CrateStorage* pCrateStorage);


uklotzde

Please add explicit to all single argument constructors. Otherwise I think we can merge this. LGTM.

daschuer · 2018-12-16T21:21:51Z

Thank you for review.

This feature was added in mixxxdj/mixxx#1798

daschuer added this to the 2.3.0 milestone Sep 2, 2018

daschuer force-pushed the 1788086 branch from faf9bc5 to b570f36 Compare September 6, 2018 19:49

Allow "" to filter for explicite empty fileds

28b0b67

daschuer force-pushed the 1788086 branch from b570f36 to 28b0b67 Compare September 6, 2018 20:50

uklotzde suggested changes Oct 3, 2018

View reviewed changes

daschuer added 6 commits October 4, 2018 21:20

NullTextFilterNode::match matches also invalid values

76a1119

Fix typo

b9e2be7

Use the first column esplicit

f54cf98

Use CATETABLE_ID

11ce5be

use a constant for ""

532c987

Improved query for tracks without crate

db56f9c

uklotzde reviewed Oct 7, 2018

View reviewed changes

daschuer added 2 commits October 8, 2018 13:58

Search for Null or Empty

b817a32

rename kExpliciteEmpty to kMissingFieldSearchTerm

dd1f85a

Merge remote-tracking branch 'upstream/master' into 1788086

defb3b8

Conflicts: src/library/searchquery.cpp

Fix building after merge

6811cf3

uklotzde reviewed Oct 19, 2018

View reviewed changes

daschuer added 2 commits October 21, 2018 22:28

use SELECT DISTINCT

fc22212

Added NullNumeric filter node

5942d22

Merge remote-tracking branch 'upstream/master' into 1788086

ef1d06a

Conflicts: src/library/searchqueryparser.cpp

uklotzde reviewed Dec 16, 2018

View reviewed changes

uklotzde approved these changes Dec 16, 2018

View reviewed changes

added explicit keyword

82cb592

daschuer merged commit 0be1a70 into mixxxdj:master Dec 16, 2018

esbrandt added a commit to mixxxdj/manual that referenced this pull request May 3, 2020

Add example for filtering empty fields

bfd29de

This feature was added in mixxxdj/mixxx#1798

esbrandt mentioned this pull request May 3, 2020

Add example for filtering empty fields mixxxdj/manual#110

Merged

daschuer deleted the 1788086 branch September 26, 2021 17:36

mixxxbot mentioned this pull request Aug 23, 2022

Show tracks without a crate #9411

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow "" to filter for explicite empty fileds #1798

Allow "" to filter for explicite empty fileds #1798

daschuer commented Sep 2, 2018

daschuer commented Sep 11, 2018

benis commented Oct 1, 2018

daschuer commented Oct 2, 2018

benis commented Oct 2, 2018

uklotzde Oct 3, 2018

daschuer Oct 4, 2018

uklotzde Oct 3, 2018

uklotzde Oct 3, 2018

uklotzde Oct 3, 2018

uklotzde Oct 3, 2018

uklotzde Oct 3, 2018

uklotzde Oct 3, 2018

uklotzde commented Oct 3, 2018

daschuer commented Oct 4, 2018

uklotzde Oct 7, 2018

uklotzde Oct 7, 2018

uklotzde commented Oct 7, 2018

daschuer commented Oct 8, 2018

uklotzde commented Oct 8, 2018

daschuer commented Oct 16, 2018

uklotzde commented Oct 17, 2018

daschuer commented Oct 17, 2018

uklotzde Oct 19, 2018

uklotzde Oct 19, 2018

uklotzde Oct 19, 2018

uklotzde commented Oct 19, 2018

daschuer commented Oct 21, 2018

daschuer commented Dec 15, 2018

uklotzde Dec 16, 2018

uklotzde Dec 16, 2018

uklotzde left a comment •

edited

Loading

daschuer commented Dec 16, 2018

Allow "" to filter for explicite empty fileds #1798

Allow "" to filter for explicite empty fileds #1798

Conversation

daschuer commented Sep 2, 2018

daschuer commented Sep 11, 2018

benis commented Oct 1, 2018

daschuer commented Oct 2, 2018

benis commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Oct 3, 2018

daschuer commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Oct 7, 2018

daschuer commented Oct 8, 2018

uklotzde commented Oct 8, 2018

daschuer commented Oct 16, 2018

uklotzde commented Oct 17, 2018

daschuer commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Oct 19, 2018

daschuer commented Oct 21, 2018

daschuer commented Dec 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde left a comment • edited Loading

Choose a reason for hiding this comment

daschuer commented Dec 16, 2018

uklotzde left a comment •

edited

Loading