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.

Sign up for GitHub

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

Allow computing the empty transitive path #1800

Closed
wants to merge 2 commits into from
Closed

Allow computing the empty transitive path #1800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2406caf
Support empty transitive path
RobinTF Feb 12, 2025
f21e561
Add Unit Tests
RobinTF Feb 12, 2025
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
2 changes: 1 addition & 1 deletion src/engine/TransitivePathBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class TransitivePathBase : public Operation {
TransitivePathSide leftSide, TransitivePathSide rightSide,
size_t minDist, size_t maxDist);

virtual ~TransitivePathBase() = 0;
~TransitivePathBase() override = 0;

/**
* Returns a new TransitivePath operation that uses the fact that leftop
Expand Down
9 changes: 1 addition & 8 deletions src/engine/TransitivePathImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class TransitivePathImpl : public TransitivePathBase {
for (auto& pair : result) {
co_yield pair;
}
};
}

protected:
/**
Expand All @@ -155,13 +155,6 @@ class TransitivePathImpl : public TransitivePathBase {
* @return Result The result of the TransitivePath operation
*/
ProtoResult computeResult(bool requestLaziness) override {
if (minDist_ == 0 && !isBoundOrId() && lhs_.isVariable() &&
rhs_.isVariable()) {
AD_THROW(
"This query might have to evaluate the empty path, which is "
"currently "
"not supported");
}
Comment on lines -158 to -164
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this is not quite correct.
The empty path must in principle contain everything, not only entities that are in some way contained in the Path with the *. This means, that we have to fix this in the TurtleParser.

auto [startSide, targetSide] = decideDirection();
// In order to traverse the graph represented by this result, we need random
// access across the whole table, so it doesn't make sense to lazily compute
Expand Down
26 changes: 17 additions & 9 deletions test/TransitivePathTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,28 +627,36 @@ TEST_P(TransitivePathTest, maxLength2ToId) {
}

// _____________________________________________________________________________
TEST_P(TransitivePathTest, zeroLengthException) {
TEST_P(TransitivePathTest, zeroLength) {
auto sub = makeIdTableFromVector({
{0, 2},
{2, 4},
{4, 7},
{0, 7},
{3, 3},
{7, 0},
// Disconnected component.
{10, 11},
});

auto expected = makeIdTableFromVector({{0, 0},
{0, 2},
{0, 4},
{0, 7},
{2, 2},
{2, 4},
{2, 7},
{4, 4},
{4, 7},
{10, 10},
{10, 11},
{7, 7},
{11, 11}});

TransitivePathSide left(std::nullopt, 0, Variable{"?start"}, 0);
TransitivePathSide right(std::nullopt, 1, Variable{"?target"}, 1);
auto T =
makePathUnbound(std::move(sub), {Variable{"?start"}, Variable{"?target"}},
left, right, 0, std::numeric_limits<size_t>::max());
AD_EXPECT_THROW_WITH_MESSAGE(
T->computeResultOnlyForTesting(requestLaziness()),
::testing::ContainsRegex("This query might have to evaluate the empty "
"path, which is currently "
"not supported"));
auto resultTable = T->computeResultOnlyForTesting(requestLaziness());
assertResultMatchesIdTable(resultTable, expected);
}

// _____________________________________________________________________________
Expand Down
Loading