-
Notifications
You must be signed in to change notification settings - Fork 34
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
fix: injected standard prefixes reference wikidata #804
Conversation
cb805bf
to
4e5589b
Compare
4e5589b
to
11fb8b2
Compare
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.
it all looks good to me :)
Unfortunately I've definitely not managed to build all the context of this ticket up in my head and I can't dedicate the time to doing it justice but let me try to ask a couple of maybe helpful questions:
Nothing blocking and perhaps nothing smart said but maybe it's of some help. :) |
Yeah, TBH I think there was never a proper specification for how this should all behave. I also wondered if the I'd actually question if "magically injecting prefixes" was ever a desirable thing from a product standpoint. It saves a tiny bit of typing or clicking a "add prefixes" button in some UI at the expense of some real mystery for the user. I skimmed the SPARQL spec for how acceptable or not it would be to have this magic insertion of prefixes and couldn't find any strong evidence for or against. |
solution has been completely changed, please have a look again.
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.
LGTM but I'm now also hoping that this doesn't impact in some way the "label" SERVICE; I wonder if you already checked this?
I was merely commenting and not requesting changes/approval) because I'd not care to tread on any toes of people not in my team :) |
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 haven't fully explored this, but to the 70% that I do grok what is happening here this seems like a clear and needed fix. The comment is appreciated. So this won't break current configuration for folks?
Thanks for catching that. I just double checked and the label service works correctly in e.g. PREFIX PP: <https://wikibase.example/prop/>
SELECT ?aLabel
WHERE
{
?a PP:P1 ?c
SERVICE wikibase:label { bd:serviceParam wikibase:language "[AUTO_LANGUAGE]". }
} |
Well, I would say: "it finally works as expected". So, "no", because I would consider this a bug fix. |
As reported in #798, default prefixes injected by WDQS did not align with theWIKIBASE_CONCEPT_URI
setting added in #771. This PR fixes it.As reported in #798, the "standard prefixes" injected by WDQS do not align with the WIKIBASE_CONCEPT_URI setting introduced in #771, nor do they match the corresponding values for Wikidata. Currently, the WDQS-injected "standard prefixes" (such as
wd:
) referencehttp://wikidata/
, which is the internal hostname of the wikibase/mediawiki container within the Docker network.Particularly in the context of federation, it is essential to maintain the "standard prefixes" for referencing Wikidata [1] [2]. To accommodate the local Wikibase instance, prefixes can be set inline, as is currently done on wikibase.cloud [2]. This pull request addresses this discrepancy and updates the "standard prefixes" to point to Wikidata. Additional context can be found in [3].
[1] https://www.mediawiki.org/wiki/Wikibase/Wikibase.cloud/First_steps#View_your_data_using_the_Query_Service
[2] https://phabricator.wikimedia.org/T335448
[3] https://phabricator.wikimedia.org/T379232