Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement cast expressions correctly #1751

Merged
merged 5 commits into from
Feb 4, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 49 additions & 2 deletions src/engine/sparqlExpressions/ConvertToNumericExpression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@
#include "engine/sparqlExpressions/NaryExpressionImpl.h"

namespace sparqlExpression {
namespace detail::string_expressions {
class StrExpressionImpl;
}
namespace detail::to_numeric {

// class that converts an input `int64_t`, `double` or `std::string`
// to a numeric value `int64_t` or `double`
template <typename T>
template <typename T, bool AllowExponentialNotation = true>
requires std::same_as<int64_t, T> || std::same_as<double, T>
class ToNumericImpl {
private:
Expand All @@ -24,7 +27,14 @@
return Id::makeFromInt(resT);
}
} else {
auto conv = absl::from_chars(strStart, strEnd, resT);
// Abseil doesn't match leading + signs, so we skip them.
if (!str.empty() && str.at(0) == '+') {
strStart++;
}

Check warning on line 33 in src/engine/sparqlExpressions/ConvertToNumericExpression.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/sparqlExpressions/ConvertToNumericExpression.cpp#L32-L33

Added lines #L32 - L33 were not covered by tests
auto conv = absl::from_chars(strStart, strEnd, resT,
AllowExponentialNotation
? absl::chars_format::general
: absl::chars_format::fixed);
if (conv.ec == std::error_code{} && conv.ptr == strEnd) {
return Id::makeFromDouble(resT);
}
Expand Down Expand Up @@ -61,9 +71,38 @@

using ToInteger = NARY<1, FV<ToNumericImpl<int64_t>, ToNumericValueGetter>>;
using ToDouble = NARY<1, FV<ToNumericImpl<double>, ToNumericValueGetter>>;
using ToDecimal =
NARY<1, FV<ToNumericImpl<double, false>, ToNumericValueGetter>>;
Comment on lines 71 to +73
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you verified, that this is indeed a subtle difference between double and decimal?
Even if, I am still not sure whether it currently is a good idea to treat them differently in this single function, when we implement them the same in all other cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I compared the SPARQL grammar for decimal and yes, this is the difference:
decimal double grammar difference

} // namespace detail::to_numeric

namespace detail::to_boolean {
class ToBooleanImpl {
public:
Id operator()(IntDoubleStr value) const {

Check warning on line 81 in src/engine/sparqlExpressions/ConvertToNumericExpression.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/sparqlExpressions/ConvertToNumericExpression.cpp#L81

Added line #L81 was not covered by tests
if (std::holds_alternative<std::string>(value)) {
const std::string& str = std::get<std::string>(value);

Check warning on line 83 in src/engine/sparqlExpressions/ConvertToNumericExpression.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/sparqlExpressions/ConvertToNumericExpression.cpp#L83

Added line #L83 was not covered by tests
if (str == "true" || str == "1") {
return Id::makeFromBool(true);
}

Check warning on line 86 in src/engine/sparqlExpressions/ConvertToNumericExpression.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/sparqlExpressions/ConvertToNumericExpression.cpp#L85-L86

Added lines #L85 - L86 were not covered by tests
if (str == "false" || str == "0") {
return Id::makeFromBool(false);
}
return Id::makeUndefined();

Check warning on line 90 in src/engine/sparqlExpressions/ConvertToNumericExpression.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/sparqlExpressions/ConvertToNumericExpression.cpp#L88-L90

Added lines #L88 - L90 were not covered by tests
} else if (std::holds_alternative<int64_t>(value)) {
return Id::makeFromBool(std::get<int64_t>(value) != 0);

Check warning on line 92 in src/engine/sparqlExpressions/ConvertToNumericExpression.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/sparqlExpressions/ConvertToNumericExpression.cpp#L92

Added line #L92 was not covered by tests
} else if (std::holds_alternative<double>(value)) {
return Id::makeFromBool(std::get<double>(value) != 0);
} else {
Comment on lines +90 to +93
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have to check again whether that is the correct/intended behavior.

AD_CORRECTNESS_CHECK(std::holds_alternative<std::monostate>(value));
return Id::makeUndefined();
}
}

Check warning on line 99 in src/engine/sparqlExpressions/ConvertToNumericExpression.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/sparqlExpressions/ConvertToNumericExpression.cpp#L94-L99

Added lines #L94 - L99 were not covered by tests
};
using ToBoolean = NARY<1, FV<ToBooleanImpl, ToNumericValueGetter>>;
} // namespace detail::to_boolean

using namespace detail::to_numeric;
using namespace detail::to_boolean;
using Expr = SparqlExpression::Ptr;

Expr makeConvertToIntExpression(Expr child) {
Expand All @@ -73,4 +112,12 @@
Expr makeConvertToDoubleExpression(Expr child) {
return std::make_unique<ToDouble>(std::move(child));
}

Expr makeConvertToDecimalExpression(Expr child) {
return std::make_unique<ToDecimal>(std::move(child));
}

Expr makeConvertToBooleanExpression(Expr child) {
return std::make_unique<ToBoolean>(std::move(child));
}

Check warning on line 122 in src/engine/sparqlExpressions/ConvertToNumericExpression.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/sparqlExpressions/ConvertToNumericExpression.cpp#L120-L122

Added lines #L120 - L122 were not covered by tests
} // namespace sparqlExpression
7 changes: 7 additions & 0 deletions src/engine/sparqlExpressions/NaryExpression.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ SparqlExpression::Ptr makeSHA256Expression(SparqlExpression::Ptr child);
SparqlExpression::Ptr makeSHA384Expression(SparqlExpression::Ptr child);
SparqlExpression::Ptr makeSHA512Expression(SparqlExpression::Ptr child);

SparqlExpression::Ptr makeConvertToStringExpression(
SparqlExpression::Ptr child);

SparqlExpression::Ptr makeIfExpression(SparqlExpression::Ptr child1,
SparqlExpression::Ptr child2,
SparqlExpression::Ptr child3);
Expand All @@ -104,6 +107,10 @@ SparqlExpression::Ptr makeIfExpression(SparqlExpression::Ptr child1,
SparqlExpression::Ptr makeConvertToIntExpression(SparqlExpression::Ptr child);
SparqlExpression::Ptr makeConvertToDoubleExpression(
SparqlExpression::Ptr child);
SparqlExpression::Ptr makeConvertToDecimalExpression(
SparqlExpression::Ptr child);
SparqlExpression::Ptr makeConvertToBooleanExpression(
SparqlExpression::Ptr child);

// Implemented in RdfTermExpressions.cpp
SparqlExpression::Ptr makeDatatypeExpression(SparqlExpression::Ptr child);
Expand Down
4 changes: 4 additions & 0 deletions src/engine/sparqlExpressions/StringExpressions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -619,4 +619,8 @@

Expr makeSHA512Expression(Expr child) { return make<SHA512Expression>(child); }

Expr makeConvertToStringExpression(Expr child) {
return std::make_unique<StrExpressionImpl>(std::move(child));
}

Check warning on line 624 in src/engine/sparqlExpressions/StringExpressions.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/sparqlExpressions/StringExpressions.cpp#L622-L624

Added lines #L622 - L624 were not covered by tests

} // namespace sparqlExpression
19 changes: 18 additions & 1 deletion src/parser/sparqlParser/SparqlQleverVisitor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,28 @@
checkNumArgs(1);
return sparqlExpression::makeConvertToIntExpression(
std::move(argList[0]));
} else if (functionName == "double" || functionName == "decimal") {
}
if (functionName == "decimal") {
checkNumArgs(1);
return sparqlExpression::makeConvertToDecimalExpression(
std::move(argList[0]));
}
// We currently don't have a float type, so we just convert to double.
if (functionName == "double" || functionName == "float") {
checkNumArgs(1);
return sparqlExpression::makeConvertToDoubleExpression(
std::move(argList[0]));
}
if (functionName == "boolean") {
checkNumArgs(1);
return sparqlExpression::makeConvertToBooleanExpression(
std::move(argList[0]));
}

Check warning on line 177 in src/parser/sparqlParser/SparqlQleverVisitor.cpp

View check run for this annotation

Codecov / codecov/patch

src/parser/sparqlParser/SparqlQleverVisitor.cpp#L174-L177

Added lines #L174 - L177 were not covered by tests
if (functionName == "string") {
checkNumArgs(1);
return sparqlExpression::makeConvertToStringExpression(
std::move(argList[0]));
}

Check warning on line 182 in src/parser/sparqlParser/SparqlQleverVisitor.cpp

View check run for this annotation

Codecov / codecov/patch

src/parser/sparqlParser/SparqlQleverVisitor.cpp#L179-L182

Added lines #L179 - L182 were not covered by tests
}

// QLever-internal functions.
Expand Down
2 changes: 1 addition & 1 deletion test/SparqlAntlrParserTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1799,7 +1799,7 @@ TEST(SparqlParser, FunctionCall) {
expectFunctionCall(absl::StrCat(xsd, "double>(?x)"),
matchUnary(&makeConvertToDoubleExpression));
expectFunctionCall(absl::StrCat(xsd, "decimal>(?x)"),
matchUnary(&makeConvertToDoubleExpression));
matchUnary(&makeConvertToDecimalExpression));

// Wrong number of arguments.
expectFunctionCallFails(absl::StrCat(geof, "distance>(?a)"));
Expand Down
Loading