fix: Duplicated hierarchies #1260
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
...we should retrieve the name of actual hierarchy, not nextInHierarchy
...as there are only ~3 legacy hierarchies on LINDAS PROD, it makes sense to remove them.
...as otherwise we also include hierachies retrieved through DESCRIBEing the cube, which are not part of the shape.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
lloydrichards
approved these changes
Nov 10, 2023
Rdataflow
reviewed
Nov 10, 2023
|
@bprusinowski LGTM 👍 |
…into fix/hierarchies-querying
ptbrowne
approved these changes
Nov 13, 2023
| const cube = await loaders.cube.load(iri); | ||
| const rawCube = await loaders.cube.load(iri); | ||
| // Currently we always default to the latest cube. | ||
| const cube = await getLatestCube(rawCube); |
There was a problem hiding this comment.
For the splitted issue, getLatestCube could be put on ExtendedCube.
| import rdf from "rdf-ext"; | ||
| import DatasetExt from "rdf-ext/lib/Dataset"; | ||
|
|
||
| export class ExtendedCube extends Cube { |
There was a problem hiding this comment.
Cna you add a docstring describing the general goal ? So that when we see a method that should be there, we can be sure that this class is the right one ?
| } | ||
| return []; | ||
|
|
||
| return uniqBy(hierarchies, (hierarchy) => getName(hierarchy, locale)); |
There was a problem hiding this comment.
For the splitted issue : getDimensionHierarchies could be on ExtendedCube. This way we will hide the complexity of the shapePtr there.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1257.
Closes #1263.
This PR:
hasHierarchy), as there is only one such hierarchy on LINDAS,Context
It looks like there's too many hierarchies fetched in the example chart from #1257 (https://visualize.admin.ch/de/v/IYdcAI0tH0Io?dataSource=Prod).
There are two issues visible in this chart:
double.selection.mov
(Apparent) duplicated hierarchies
First issue occurs due to retrieving name from
hierarchyPointer.out(ns.cubeMeta.nextInHierarchy)instead ofhierarchyPointerdirectly. @ptbrowne do you remember the context here (it looks like this has been done to avoid duplication)? After looking at docs and investigating the data on LINDAS, I think we should base directly onhierarchyPointer– if we do it, the name of hierarchies are unique (Schweiz - Produktionsregion - WirtschaftsregionandSchweiz - Produktionsregion).Hierarchy that (most probably) shouldn't be fetched at all
Second one is more weird – generally, when we fetch a cube, two queries are fired, one for cube and the other one for cube shape. It looks like this behavior results in more hierarchies that we're actually interested in, as (I think, also based on kronmar's query), we should only fetch hierarchies that are specified in the cube's shape.
I didn't investigate enough to say for sure why these additional hierarchies are being fetched along with
cube, but if we just fetchcube shape, it retrieves correct hierarchies.It's a bit unfortunate, as basically we need to fetch shape twice, once initially, and afterwards again when fetching hierarchies, as it looks we can't distinguish if a given triple came fromcubeorshape. Some optimisation might be possible, but I think it would need to be somehow connected with changes tocube-view-querylibrary, to be able to tell if a given triple came from one or the other query.I introduced an
ExtendedCubeclass to allow us to store shape triples insideshapePtr, thus removing a need of fetching the shape twice. It might be useful to introduce such improvement directly tocube-view-querylibrary, or to better understand why there is hierarchy mismatch when fetching cube and shape.cc @Rdataflow