SearchQueryParser: Add "OR" search operator

[fwcd]

Fixes #8881

This adds | and (uppercase) OR as syntax for the OR operator in search queries. Thus queries such as house | disco or equivalently house OR disco will now yield the combined results from querying house and disco.

Both | and OR have strong precedent, e.g. with Google using these operators¹. Especially | is terse, easy to read/write and familiar to technical users.

Footnotes

1. https://ahrefs.com/blog/google-advanced-search-operators ↩

[daschuer]

Thank you. Can you also ass a test for this?

[fwcd]

I've updated the parser to handle quotes correctly (i.e. title:"some | title" | house, for example, will now behave as expected) and added some tests to verify that this works correctly.

[fwcd]

I don't understand why the tests are failing in CI, they work on my machine™. Off all the tests, SearchQueryParserTest.EmptyOrOperator, whose implementation is rather trivial, seems to fail due to an interrupt. Any ideas?

[fwcd]

Couldn't reproduce the test failure in an Arch Linux Docker container, neither on arm64 nor on x86_64.

[fwcd]

Ah, I see, CI ran into a debug assert which I didn't get since I was building in RelWithDebInfo mode:

critical [0x55a1d4186d30] DEBUG ASSERT: "!m_nodes.empty()" in function virtual bool OrNode::match(const TrackPointer&) const at /home/runner/work/mixxx/mixxx/src/library/searchquery.cpp:127

mixxx/src/library/searchquery.cpp

Lines 125 to 132 in 9e411ff

	// An empty OR node would always evaluate to false
	// which is inconsistent with the generated SQL query!
	VERIFY_OR_DEBUG_ASSERT(!m_nodes.empty()) {
	// Evaluate to true even if the correct choice would
	// be false to keep the evaluation consistent with
	// the generated SQL query.
	return true;
	}

Might be an interesting question how we should handle this. Apart from the top-level AndNode, empty queries will simply not produce a node at all and queries such as | produce an (empty) OrNode. IMO an empty OrNode, even if it's a bit odd, shouldn't be a semantic error and simply not match anything. Could we update the SQL generation to reflect that?

[daschuer]

I think we should consider the intermediate results when typing. I can imagine that showing nothing because of a semantic error feels bad. Is this related here? I have not tested. What happen if you type "ABBA |" ?

The other issue is that we should only assert impossible conditions. Sincea user can trigger it, it must not be an assertion.

[fwcd]

What happen if you type "ABBA |"?

Empty children are currently filtered out, so this would be equivalent to simply typing ABBA:

mixxx/src/library/searchqueryparser.cpp

Lines 292 to 296 in 93b96bc

	for (const QString& rawAndNode : rawAndNodes) {
	if (!rawAndNode.isEmpty()) {
	pQuery->addNode(parseAndNode(rawAndNode));
	}
	}

The only real edge case where the user would end up with an empty OrNode would e.g. be |, | | etc. Probably a contrived example, but we wouldn't want to hit this debug assert at all, ideally.

I'd be fine with specifying the empty OR as meaning "match everything", even if it deviates from the common convention to define it as "match nothing" if that makes SQL generation more convenient and/or lines up with what the user expects (i.e. still showing all tracks, when the query is | and thus empty-ish).

The (logically sound) alternative would be to define the empty OR as matching nothing and emit FALSE as SQL. I'd probably prefer that, since it would only show up in edge cases anyway (see above) and be consistent with boolean logic elsewhere.

We should remove this debug assert either way. Wdyt?

[JoergAtGithub]

we could even support both | and OR, which is what Google does

I think we should support both!

[fwcd]

@JoergAtGithub I've had a go at that, but parsing it seems to be less trivial than I would've hoped for, particularly the tests around literal or still fail: 3e70205

[fwcd]

Both | and OR are now supported and I've added a bunch of tests to make sure that especially the latter is handled correctly with all of its edge cases (e.g. different casing or adjacent letters shouldn't cause it to be parsed as an operator).

Additionally, I have implemented the empty-OR-as-false interpretation, which essentially amounted to removing the debug assert and generating FALSE as SQL code if there are no child nodes. As explained in #12061 (comment) this is the logically sound way to handle this and, in practice, only affects edge cases anyway (so unless the user e.g. searches |, i.e. the empty OR operator, the results table isn't necessarily empty).

[daschuer]

This works like a charm now and the code LGTM as well.
Special thanks for the extra efforts in testing.

[Be-ing]

Thanks for implementing my 6 year old feature request.

[ronso0]

I'm wondering: i don't spot any changes to user-facing strings (tr), what was the reason to target 2.5 instead of 2.4?
Do you think it would be worth backporting it to 2.4?

[fwcd]

My general philosophy was to target main for any non-trivial new features so we can focus on improving stability in 2.4 while giving these changes some time for testing. Since this isn't a huge change, I don't have strong feelings here, but it does change the parsing logic of search queries a bit.