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-1778: List graphs without count in Fuseki UI #1792

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nichtich
Copy link

@nichtich nichtich commented Mar 9, 2023

Modified the Fuseki UI to show a list of graphs without triple counts in /edit view by default because counting all graphs is slow and the /edit interface becomes unusable for large numbers for graphs & triples.

GitHub issue resolved #1778


  • Tests are included
  • Documentation change and updates are provided for the Apache Jena website
  • Commits have been squashed to remove intermediate development commit messages.
  • Key commit messages start with the issue number (GH-xxxx or JENA-xxxx)

By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.


See the Apache Jena "Contributing" guide.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Are you planning to add tests, @nichtich ? WDYT of this change @afs? I rarely use this feature, TBH. If it's OK to drop the counts, we can go ahead and finish the change with tests & docs.

@nichtich
Copy link
Author

I created no tests because the current UI was not covered by tests anyway. Same applies to documentation for this feature, it just does not apply to this pull request, so I did not check both checkboxes.

@kinow
Copy link
Member

kinow commented Mar 17, 2023

Got it. Docs are all good. First we migrated the old JQuery and Bootstrap.js code to Vue.js and then added initial unit and e2e tests. Future changes after that are expected to have tests (recent previous pull requests to the UI probably have examples of that).

But let's wait for others first to see what they think of this change. And writing these tests can be a bit boring if uou are not familiar with the test harness and tooling. In tuat case just let us know and we can send a diff or commit with some tests to this PR 👍

Copy link
Member

@rvesse rvesse left a comment

Choose a reason for hiding this comment

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

@kinow Some tests around this would be nice, I tried this feature the other day exploring a dataset and wasn't clear to me that it works at all. I think not counting triples by default is a good behavioural change as it doesn't scale well for large datasets.

Even just listing graphs may not scale well in all cases e.g. #1655

Modified the Fuseki UI to show a list of graphs without triple counts in
/edit view by default because counting all graphs is slow and the /edit
interface becomes unusable for large numbers for graphs & triples.
@nichtich
Copy link
Author

I tried to add a basic test (force-push to my fork) but not having dug into Vue testing before I'm stuck with

[Vue warn]: Failed to resolve component: router-link

and I'm not sure whether the services watcher is actually called in the testing environment and what value it should have.

@kinow
Copy link
Member

kinow commented Mar 17, 2023

I tried to add a basic test (force-push to my fork) but not having dug into Vue testing before I'm stuck with

No worries @nichtich , thanks for providing the initial structure. It's a bit annoying to write these tests without having an existing test to copy (as you noticed, things are not very intuitive, and the tests being added later means you have nothing to base your work on). I can finish the test and ping you if I need help to test/cover some part of your code 👍

Thanks!

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Change looks good to me. Will suggest a few modifications in the code (in a separate commit) and then we can discuss after that. @nichtich thanks for the PR!

} else {
const graphs = await this.$fusekiService.listGraphs(this.datasetName, endpoint)
this.graphs = Object.fromEntries(graphs.map(name => [name, ""]))
}
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 try to split this function into two later 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it looks like one function with a bool=true returns just the count, and for bool=false the graphs (with the counts too?). I think one is bringing the count to display the graphs and the count. The other is bringing the graphs and services available. If so, we can probably split this function into two, one for the graph services, and another for the counts. (Need to confirm)

this.loadingGraphs = true
this.loadingGraph = true
this.code = ''
this.selectedGraph = ''
try {
this.graphs = await this.$fusekiService.countGraphsTriples(this.datasetName, this.services.query['srv.endpoints'][0])
const endpoint = this.services.query['srv.endpoints'][0]
Copy link
Member

Choose a reason for hiding this comment

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

#1679 has some changes for the FusekiService and the src.endpoints. Once that one is merged, I will rebase here and update the code to that calls $fusekiService methods.

@kinow
Copy link
Member

kinow commented Mar 10, 2024

I noticed that most PR's were Fuseki-UI, blocked by my. I am sorry 🙇 Looking at this one now.

@kinow
Copy link
Member

kinow commented Mar 10, 2024

(PR was opened from gbv/ without allowing contributors to modify it, so I cannot rebase it or add commits with tests - ping @nichtich )

query: DATASET_GRAPHS
}
})
return ["default", ...result.data.results.bindings.map(binding => binding.g.value)]
Copy link
Member

Choose a reason for hiding this comment

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

Cleaner than the other methods doing the forEach! Will apply that to the rest of the code in a future PR. Thanks!

@@ -22,6 +22,7 @@ import { BUS } from '@/events'

const DATASET_SIZE_QUERY_1 = 'select (count(*) as ?count) {?s ?p ?o}'
const DATASET_SIZE_QUERY_2 = 'select ?g (count(*) as ?count) {graph ?g {?s ?p ?o}} group by ?g'
const DATASET_GRAPHS = 'select ?g {graph ?g {}}'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can rename this to $same_QUERY...

} else {
const graphs = await this.$fusekiService.listGraphs(this.datasetName, endpoint)
this.graphs = Object.fromEntries(graphs.map(name => [name, ""]))
}
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it looks like one function with a bool=true returns just the count, and for bool=false the graphs (with the counts too?). I think one is bringing the count to display the graphs and the count. The other is bringing the graphs and services available. If so, we can probably split this function into two, one for the graph services, and another for the counts. (Need to confirm)

expect(component.find('.card-body h3').text()).to.equal('Available Graphs')

// TODO: should return 1 instead
// expect(component.findAll('tr').length).to.equal(1)
Copy link
Member

Choose a reason for hiding this comment

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

👍

@afs
Copy link
Member

afs commented Mar 10, 2024

+1 to listing the graph without counts.

@kinow
Copy link
Member

kinow commented Mar 10, 2024

Alright, I have a local version that contains the function split into two, and I think I understood more about the goal of this issue (also spotted that I didn't add pagination to Edit, only to Info view).

@afs there are a few things to confirm on this PR.

  • So this PR disables the COUNT(*) queries for the graphs by default
  • It eagerly loads the list of graphs once you open the Edit view (even though we still have the button to count graphs)
    • What would be the best?
      • a. load all graphs by default, without the count once the page is loaded, wait for user to click to count all graphs?
      • b. load all graphs by default, without the count once the page is loaded, do not let users click on count (go to Info view instead)
      • c. load nothing by default, wait for user to click and then load graphs without count
  • Even though we are removing the COUNT((*), that's still required... when you edit the graph, the UI uses that COUNT to compare against MAX_EDITABLE_SIZE, set in the code to 10000 (from old Fuseki UI)
    • Should we let users try to save very large graphs and possibly overload UI and/or backend? i.e. drop the limit/need for count
    • Or should we load the count when the user tries to save it (or when s/he clicks on it?

Some screenshots from the current version in this PR:

(when you open the Edit view, it queries the backend when the page is loaded)

image

(when you click count triples, it's the same as main branch)

image

@kinow
Copy link
Member

kinow commented Mar 10, 2024

I think this PR is pending some more discussion, and not yet ready to be merged even adding the tests. But good progress today 👋

@kinow kinow marked this pull request as draft March 26, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fuseki UI: /edit view should not count number of triples for each graph
4 participants