Enable pre-filtering (using PrefilterExpressionIndex) of Date values over expression YEAR().
#1820
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1820 +/- ##
==========================================
- Coverage 90.18% 90.16% -0.02%
==========================================
Files 400 400
Lines 38440 38509 +69
Branches 4306 4318 +12
==========================================
+ Hits 34666 34722 +56
- Misses 2479 2483 +4
- Partials 1295 1304 +9 ☔ View full report in Codecov by Sentry. |
| } | ||
| throw std::runtime_error( | ||
| "Pre-filtering DATETIME/DATE values over YEAR failed: requires " | ||
| "INTEGER reference value."); |
There was a problem hiding this comment.
You can also output the illegal value that was passed, to make the error message more useful.
| const auto* variableExpr = dynamic_cast<const VariableExpression*>(child0); | ||
| if (!variableExpr) { | ||
| return {}; | ||
| const auto getOptVariableAndIsYear = [](const SparqlExpression* child) |
There was a problem hiding this comment.
Add a small comment about what this does.
And this lambda doesn't capture, so it can be a free function (less cluttering in the code).
| [&getOptVariableAndIsYear]( | ||
| const SparqlExpression* child0, const SparqlExpression* child1, | ||
| bool reversed) -> std::vector<PrefilterExprVariablePair> { | ||
| if (auto optVariableIsYearPair = getOptVariableAndIsYear(child0); | ||
| optVariableIsYearPair) { | ||
| const auto& [variable, prefilterDate] = optVariableIsYearPair.value(); | ||
| const auto& optReferenceValue = | ||
| detail::getIdOrLocalVocabEntryFromLiteralExpression(child1); | ||
| if (!optReferenceValue.has_value()) { | ||
| return {}; | ||
| } | ||
| return prefilterExpressions::detail::makePrefilterExpressionVec<comp>( | ||
| optReferenceValue.value(), variable, reversed, prefilterDate); |
There was a problem hiding this comment.
Simpler code flow:
auto opt1 = getOpVar(...);
if (! opt1) return {};
auto opt2 = getReferenceValue();
if (!opt2) return {};
// return the actual thing, but no double nested if statements in the code anymore
| [[maybe_unused]] size_t childIndex) const { | ||
| return std::nullopt; |
There was a problem hiding this comment.
I think we have a virtual function children() which can be used here. The default of nullopt is really not necessary, but I think you can even make it non-virtual or final , because it directly relies on the correcteness of a function we already have.
Also, this function can easily be tested.
| @@ -60,6 +60,13 @@ class SparqlExpression { | |||
| virtual std::unique_ptr<SparqlExpression> replaceChild( | |||
| size_t childIndex, std::unique_ptr<SparqlExpression> newExpression); | |||
|
|
|||
| // Returns the child at index `childIndex` for this expression. | |||
| // By default the implementation of this method returns `std::nullopt`. | |||
There was a problem hiding this comment.
See above, I don't like the default and don't think it's necessary.
Conformance check passed ✅No test result changes. |
|
No description provided.