-
Notifications
You must be signed in to change notification settings - Fork 3
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
perf: Optimize Browse page queries #1236
Changes from 1 commit
6143c45
c82f421
5621193
4ed5fdf
3d23a44
a6220e5
ea5501e
6e39852
8a11fb2
f8fbe2f
fbba91b
467a17a
15d0b76
e4c0278
a3b4a8f
ac5e6ed
3143679
91a298c
826ebeb
a2b4aef
bde860c
d1eff45
4f65f10
a726792
95341b1
986e842
7b9c508
4b9e50e
d16b2b9
6b1cedc
41353f6
c02a7d2
7631f7d
e2de8ca
3ed4efc
a01b3f7
e90b2a2
b4e87bc
d8c50c2
8d87a82
4652385
e708814
d4117a6
67c6bdc
056c640
7815ab2
0497a97
edb4e91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,7 +121,7 @@ export const searchCubes = async ({ | |
?.filter((x) => x.type === "DataCubeOrganization") | ||
.map((v) => v.value) ?? []; | ||
|
||
const scoresQuery = SELECT` | ||
let scoresQuery = SELECT` | ||
?iri ?title ?status ?datePublished ?description ?publisher ?creatorIri ?creatorLabel | ||
(GROUP_CONCAT(DISTINCT ?themeIri; SEPARATOR="${GROUP_SEPARATOR}") AS ?themeIris) (GROUP_CONCAT(DISTINCT ?themeLabel; SEPARATOR="${GROUP_SEPARATOR}") AS ?themeLabels) | ||
(GROUP_CONCAT(DISTINCT ?subthemeIri; SEPARATOR="${GROUP_SEPARATOR}") AS ?subthemeIris) (GROUP_CONCAT(DISTINCT ?subthemeLabel; SEPARATOR="${GROUP_SEPARATOR}") AS ?subthemeLabels) | ||
|
@@ -152,7 +152,7 @@ export const searchCubes = async ({ | |
)} | ||
} | ||
} | ||
${makeInFilter("creatorIri", creatorValues)} | ||
${creatorValues.length ? makeInFilter("creatorIri", creatorValues) : ""} | ||
|
||
OPTIONAL { | ||
?iri ${ns.dcat.theme} ?themeIri . | ||
|
@@ -166,7 +166,6 @@ export const searchCubes = async ({ | |
})} | ||
} | ||
} | ||
${makeInFilter("themeIri", themeValues)} | ||
|
||
# Add more subtheme termsets here when they are available | ||
${ | ||
|
@@ -238,11 +237,16 @@ export const searchCubes = async ({ | |
)` | ||
ptbrowne marked this conversation as resolved.
Show resolved
Hide resolved
bprusinowski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
: "" | ||
} | ||
|
||
`.GROUP().BY`?iri`.THEN.BY`?title`.THEN.BY`?status`.THEN.BY`?datePublished` | ||
.THEN.BY`?description`.THEN.BY`?publisher`.THEN.BY`?creatorIri`.THEN | ||
.BY`?creatorLabel`.prologue`${pragmas}`; | ||
|
||
if (themeValues.length) { | ||
scoresQuery = scoresQuery.HAVING`${themeValues | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bprusinowski what was the issue with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we removed organizations and themes queries, when we filter by e.g. Administration theme, we still need to retrieve all themes attached to a given cube, not only the Administration theme, as this would distort the left filter panel – that's why we can't use FILTER, but rather retrieve all themes, and only filter by concatenated string afterwards 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But it doesn't look like it affected the performance in any noticeable way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, you don't want to loose the other themes on the selected cubes, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, you are right – I updated the query 🎉 |
||
.map((iri) => `CONTAINS(LCASE(?themeIris), LCASE("${iri}"))`) | ||
.join(" || ")}` as any; | ||
} | ||
|
||
const scoreResults = await scoresQuery.execute(sparqlClient.query, { | ||
operation: "postUrlencoded", | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curiosity : Why do we need another graph for themes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better for the performance if we narrow down the graph, so it doesn't need to look everywhere to find the things? @Rdataflow might have a better understanding here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bprusinowski well understood 😄 it's about narrowing down the lookup to one
GRAPH
(a.k.a. one partition of the SPARQL database)