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

HDT format identifier uses only HDT version. #43

Merged
merged 1 commit into from
Dec 9, 2016

Conversation

RubenVerborgh
Copy link
Member

@RubenVerborgh RubenVerborgh commented Dec 5, 2016

Issue: HDT files produced by the current master cannot be read by other HDT libraries, nor an earlier version of this library, and unnecessarily so.

Starting with 1e23041, HDT files were being written with a format IRI of http://purl.org/HDT/hdt#HDTv1-1-2 . This unnecessarily breaks compatibility with existing HDT libraries, which expect the version URL to be http://purl.org/HDT/hdt#HDTv1. Since the version number indicates HDT_version.index_version.release, only the first component matters to the HDT file itself. This commit reverts the version IRI to http://purl.org/HDT/hdt#HDTv1, restoring backward compatibility with other HDT libraries. It also reverts the version detection code to simple equality.

Partially reverts 1e23041.
Related to #36.
Fixes bug reported in LinkedDataFragments/Client.js#26 (comment).

Starting with 1e23041, HDT files were being written
with a format IRI of http://purl.org/HDT/hdt#HDTv1-1-2.
This unnecessarily breaks compatibility with existing HDT libraries,
which expect the version URL to be http://purl.org/HDT/hdt#HDTv1.
Since the version number indicates HDT_version.index_version.release,
only the first component matters to the HDT file itself.
This commit reverts the version IRI to http://purl.org/HDT/hdt#HDTv1,
restoring backward compatibility with other HDT libraries.
It also reverts the version detection code to simple equality.

Partially reverts 1e23041.
@mielvds
Copy link
Member

mielvds commented Dec 6, 2016

Ok, looks good to me. But shouldn't we have URIs reflecting index and version as well reflected in the header?

@RubenVerborgh
Copy link
Member Author

RubenVerborgh commented Dec 6, 2016 via email

@mielvds
Copy link
Member

mielvds commented Dec 6, 2016

So there is no link between the HDT and the index at all? Everything is done by the software?

@RubenVerborgh
Copy link
Member Author

I don't think any such link should exist, given the versioning scheme, so no as far as i'm concerned. What do you think?

@RubenVerborgh
Copy link
Member Author

@mielvds Is this one okay to merge? If so, I would merge and release after (well, after we've determined the version number).

@mielvds
Copy link
Member

mielvds commented Dec 9, 2016

Well agreed, but I want to make sure first: if we did the FM index thing, did that have to be decided at HDT generation time?

@RubenVerborgh
Copy link
Member Author

RubenVerborgh commented Dec 9, 2016

Yes, but for clarity, these are two different indexes. There is the .hdt.index file, which I argue should not be in the .hdt file because they are not coupled, and there is the index of literals (a dictionary), which is inside of the .hdt file and is thus coupled to it.

@mielvds mielvds merged commit c0ded75 into master Dec 9, 2016
@mielvds
Copy link
Member

mielvds commented Dec 9, 2016

done.

@RubenVerborgh RubenVerborgh deleted the fix-backward-compatibility branch December 9, 2016 09:06
@mielvds mielvds modified the milestone: Early 2017 Dec 9, 2016
@webdata
Copy link
Contributor

webdata commented Dec 9, 2016

Hi, I also think we don't have to include the index version in the HDT dataset. Another discussion is if we should allow the OPTION to have the index within a single HDT file (suggested by Wouter, #7 (comment)). I think we could allow this option with backward compatibility by just appending the index at the end of the file and marking this in the preamble. But this is something to discuss probably after this upcoming release

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.

3 participants