Order by aggregate #76
Conversation
e2e/scientists_queries.yaml
Outdated
| - num_cols: 1 | ||
| # -num_rows: 5295 # greater than current limit | ||
| - selected: ["?place"] | ||
| - res: [["<United_States_of_America>"]] |
There was a problem hiding this comment.
😂 nice trick to test for ordering, I think we really should add an order check function to queryit.py. I guess to support diferent types we could just make that explicit in the checks name
There was a problem hiding this comment.
This is now implemented in PR #77
There was a problem hiding this comment.
After trying to use the new order checks for this I realized that the e2e testing framework doesn't actually have the data it needs to check the ordering, as that is not selected and thus never returned by the server. As a still somewhat hackish solution I also added the aggregates to the select part of the query, with a different result variable name, which the e2e testing framework can then use to check the order.
src/parser/ParsedQuery.cpp
Outdated
| for (size_t i = 0; i < _orderBy.size(); i++) { | ||
| OrderKey& key = _orderBy[i]; | ||
| if (key._key[0] == '(') { | ||
| std::string inner = key._key.substr(1, key._key.size() - 2); |
src/util/StringUtils.h
Outdated
| @@ -110,6 +110,14 @@ inline vector<string> split(const string& orig, const char sep); | |||
| //! Splits a string at any maximum length sequence of whitespace | |||
| inline vector<string> splitWs(const string& orig); | |||
|
|
|||
| //! Splits a string at any maximum length sequence of whitespace, ignoring | |||
| //! whitespace withing an escaped sequence. Left char begins an escaped | |||
test/StringUtilsTest.cpp
Outdated
| TEST(StringUtilsTest, splitWsWithEscape) { | ||
| setlocale(LC_CTYPE, ""); | ||
| string s1 = " a1 \t (this is() one) two (\t\na)()"; | ||
| auto v1 = splitWsWithEscape(s1, '(', ')'); |
There was a problem hiding this comment.
This only tests the left != right case not the left == right one.
src/util/StringUtils.h
Outdated
| pos++; | ||
| } | ||
| // avoid adding whitespace at the back of the string | ||
| // if (!::isspace(static_cast<unsigned char>(orig[orig.size() - 1]))) { |
| ?x <Place_of_birth> ?place . | ||
| } | ||
| GROUP BY ?place | ||
| ORDER BY DESC((COUNT(?x) as ?count)) |
There was a problem hiding this comment.
Maybe we should also test another aggregate like AVG?
Also added another test for splitWsWithEscape when left == right.
floriankramer
force-pushed
the
order_by_aggregate
branch
from
ea30f8e
to
8cdcd28
Compare
Also fixed the e2e tests.
floriankramer
force-pushed
the
order_by_aggregate
branch
from
8cdcd28
to
584806a
Compare
This pr renames has-relation to has-predicate both when parsing sparql queries as well as in the code (e.g. in
HasRelationScanwhich is nowHasPredicateScan).All index functions that require a loaded patterns file now throw an exception if no patterns file was loaded.
Aggregate aliases can now be used inside the ORDER BY clause without having to be selected, as the sparql standard demands.