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

GH-5030: fix FedXDataset support for queries with FROM clauses #5031

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

aschwarte10
Copy link
Contributor

@aschwarte10 aschwarte10 commented Jun 14, 2024

GitHub issue resolved: #5030

FedX supports interpret the Dataset being defined on the query: if it is of type FedXDataset, the federation is reduced to the members defined by FedXDataset#getEndpoints().

This is a nice means to evaluate a query on a subset of the federation and is generally working fine.

When the query contains a FROM clause, however, the externally passed Dataset is wrapped into a FallbackDataset (which internally keeps the FedXDataset as primary one.

This change now inspects the FallbackDataset and unwraps it to reduce the members. Note that for execution still the "FallbackDataset" is used.


PR Author Checklist (see the contributor guidelines for more details):

  • my pull request is self-contained
  • I've added tests for the changes I made
  • I've applied code formatting (you can use mvn process-resources to format from the command line)
  • I've squashed my commits where necessary
  • every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

FedX supports interpret the Dataset being defined on the query: if it is
of type FedXDataset, the federation is reduced to the members defined by
FedXDataset#getEndpoints().

This is a nice means to evaluate a query on a subset of the federation
and is generally working fine.

When the query contains a FROM clause, however, the externally passed
Dataset is wrapped into a FallbackDataset (which internally keeps the
FedXDataset as primary one.

This change now inspects the FallbackDataset and unwraps it to reduce
the members. Note that for execution still the "FallbackDataset" is
used.
@@ -113,4 +113,12 @@ private void appendURI(StringBuilder sb, IRI uri) {
}
}

public Dataset getPrimary() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you think it is OK to make the primary dataset accessible for use in FedX?

FedX fed = federationContext.getFederation();
members = fed.getMembers();
}
List<Endpoint> members = getMembersFromContext(stats);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: the logic is now moved into a helper method. Potentially allowing derived extensions to do additional pruning of members in the query context


// if the query uses a FROM clause, the external dataset
// is wrapped as primary dataset in FallbackDataset
if (queryDataset instanceof FallbackDataset) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is actually the only change in logic: we unwrap the primary dataset from the FallbackDataset

@aschwarte10
Copy link
Contributor Author

@hmottestad can I ask for a review to have this bug fix still in 5.0? It is relevant for the use of FedX in our product, where we have means to reduce the federation for a specific query.

Also: can you comment on the failed "dash-license" check? Is this something we need to look after? In my change I have not done anything with dependencies.

@hmottestad hmottestad merged commit 6413b02 into main Jun 17, 2024
8 of 9 checks passed
@hmottestad hmottestad deleted the GH-5030-fedx-dataset-fallback branch June 17, 2024 13:20
@hmottestad
Copy link
Contributor

Dunno why we are seeing that license issue now. We've had some random issues over the past few months were the license tool suddenly starts flagging an existing dependency without any changes from our side.

I've added the three packages to harvet on clearlydefined.io so let's hope that that works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FedX does not correctly reduce federation members, when the query contains a FROM clause
2 participants