Don't drop `FILTER` operations with unbound variables #2103

joka921 · 2025-06-04T09:54:29Z

So far, QLever silently dropped FILTER operations that referred to variables that were unbound in the respective group graph pattern. This is now fixed. For example, the following query now has an empty result as it should:

SELECT * {
  BIND (3 as ?a)
  FILTER (?a < ?b)
}

Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

First feed this to the conformance tests, then write unit tests. Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

RobinTF · 2025-06-04T10:44:38Z

@joka921 Can you check if this also fixes #1997 by any chance? I believe it is the same kind of issue.

codecov · 2025-06-04T11:05:59Z

Codecov Report

Attention: Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.19%. Comparing base (641dc74) to head (ff78e9e).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
src/engine/QueryPlanner.cpp	94.44%	0 Missing and 1 partial ⚠️
src/engine/idTable/IdTable.h	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2103      +/-   ##
==========================================
- Coverage   91.20%   91.19%   -0.01%     
==========================================
  Files         434      434              
  Lines       42609    42617       +8     
  Branches     4805     4807       +2     
==========================================
+ Hits        38860    38866       +6     
  Misses       2348     2348              
- Partials     1401     1403       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

RobinTF

The code looks good mostly, but I'd like to see an explanation for me to understand the changes a little better

test/IdTableTest.cpp

test/UnionTest.cpp

RobinTF · 2025-06-04T20:44:23Z

src/engine/QueryPlanner.cpp

 void QueryPlanner::applyFiltersIfPossible(
    vector<SubtreePlan>& row, const vector<SparqlFilter>& filters) const {
+  static_assert(!alsoApplyIfMissingVariables || replace);


If these options are somewhat exclusive, wouldn't it make sense to have an enum with 3 modes instead?
Also what I haven't understood, why is this even a template parameter in the first place? Does this justify the compilation overhead?

RobinTF · 2025-06-04T20:49:58Z

test/UnionTest.cpp

@@ -185,6 +185,43 @@ TEST(Union, ensurePermutationIsAppliedCorrectly) {
  }
 }

+// _____________________________________________________________________________
+TEST(Union, inputWithZeroColumns) {


This is of course an important edge case to consider, but I don't get how this is related to unbound variables. Please enlighten me

# Conflicts: # test/QueryPlannerTest.cpp

Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

RobinTF

Thank you very much

Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

hannahbast

Thanks a lot for this fix! I tested this and it worked fine. I slightly revised the title and the description. In QueryPlanner.cpp:1338 not all possible outcomes are covered by tests yet. Is this a regression, @joka921, or was this already the case in the previous code?

joka921 · 2025-06-17T12:49:50Z

@hannahbast
That is no regression, rather an artifact of the way the coverage tool works.
Feel free to merge this.

hannahbast

According to @joka921, the CodeCov complaint was an artifact, so I will merge this now.

sonarqubecloud · 2025-06-17T15:17:57Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

sparql-conformance · 2025-06-17T15:40:29Z

Conformance check failed ❌

Test Status Changes 📊

Number of Tests	Previous Status	Current Status
4	Passed	Failed
1	Failed	Passed

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=ff78e9e583153afd3e8b03f6fef33f601bfb1ea7&prev=4f9b0085a2573c856ae848d512f2667434d022ff

joka921 added 3 commits June 4, 2025 11:44

Fix the ignoring of filters.

37977c5

Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

Fix the unit tests.

d9293b4

First feed this to the conformance tests, then write unit tests. Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

Fix the unit tests.

55b6960

First feed this to the conformance tests, then write unit tests. Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

joka921 marked this pull request as ready for review June 4, 2025 17:54

RobinTF reviewed Jun 4, 2025

View reviewed changes

joka921 added 2 commits June 17, 2025 07:24

Merge branch 'master' into applyAllFiltersAtTheEnd

7471bb4

# Conflicts: # test/QueryPlannerTest.cpp

Apply the changes from Robins review.

7492a2b

Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

RobinTF approved these changes Jun 17, 2025

View reviewed changes

Fix the compilation again.

ff78e9e

Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

hannahbast changed the title ~~Don't drop filters with unbound variables~~ Don't drop FILTER operations with unbound variables Jun 17, 2025

hannahbast requested changes Jun 17, 2025

View reviewed changes

hannahbast approved these changes Jun 17, 2025

View reviewed changes

hannahbast merged commit b8517f5 into ad-freiburg:master Jun 17, 2025
18 of 19 checks passed

RobinTF mentioned this pull request Jun 17, 2025

incorrect treatment of unbound variables in FILTER #1997

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Don't drop `FILTER` operations with unbound variables #2103

Don't drop `FILTER` operations with unbound variables #2103

Uh oh!

joka921 commented Jun 4, 2025 •

edited by hannahbast

Loading

Uh oh!

RobinTF commented Jun 4, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jun 4, 2025 •

edited

Loading

Uh oh!

RobinTF left a comment

Uh oh!

Uh oh!

Uh oh!

RobinTF Jun 4, 2025

Uh oh!

RobinTF Jun 4, 2025

Uh oh!

RobinTF left a comment

Uh oh!

hannahbast left a comment

Uh oh!

joka921 commented Jun 17, 2025

Uh oh!

hannahbast left a comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Jun 17, 2025

Uh oh!

sparql-conformance bot commented Jun 17, 2025

Uh oh!

Uh oh!

Don't drop FILTER operations with unbound variables #2103

Don't drop FILTER operations with unbound variables #2103

Uh oh!

Conversation

joka921 commented Jun 4, 2025 • edited by hannahbast Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RobinTF commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RobinTF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RobinTF Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

RobinTF Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

RobinTF left a comment

Choose a reason for hiding this comment

Uh oh!

hannahbast left a comment

Choose a reason for hiding this comment

Uh oh!

joka921 commented Jun 17, 2025

Uh oh!

hannahbast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Jun 17, 2025

Quality Gate passed

Uh oh!

sparql-conformance bot commented Jun 17, 2025

Conformance check failed ❌

Test Status Changes 📊

Uh oh!

Uh oh!

Don't drop `FILTER` operations with unbound variables #2103

Don't drop `FILTER` operations with unbound variables #2103

joka921 commented Jun 4, 2025 •

edited by hannahbast

Loading

RobinTF commented Jun 4, 2025 •

edited

Loading

codecov bot commented Jun 4, 2025 •

edited

Loading