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

Value Compaction broken #171

Closed
gkellogg opened this issue Oct 21, 2012 · 5 comments
Closed

Value Compaction broken #171

gkellogg opened this issue Oct 21, 2012 · 5 comments

Comments

@gkellogg
Copy link
Member

Back in June, I made a comment on a change made by @dlongley to Value Compaction in commit fdb3371. There was a small change made, but I think it's still seriously wrong.

Step 1 was updated slightly, but it now says the following:

If value only has one property and the active context has no default language, then the compacted value is the value of @value.

Note that this doesn't look at term coercion at all.

Step 4 indicates that {"@id": "foo"} can always be compacted to the IRI Compacted form of "foo", which is only true if the active property is coerced to @id.

Otherwise, if_ value_ contains an @id key, the compacted value is_ value_ with the value of @id processed according to the IRI Compaction steps.

There is no provision for native values, except in the last step.

This came to light, as I was tracking down a bug in compact-0018 where the expected value in a list of term5 is {"@value": 4}, not just 4. IMO, it should be just 4, as it has unambigious meaning, and doesn't need to be represented in value form.

Value compaction will require other changes to handle language conatiner terms, but I don't understand how it could be so far off, and not match the rest of the compaction tests.

@lanthaler
Copy link
Member

This came to light, as I was tracking down a bug in compact-0018 where the expected value in a list of term5 is {"@value": 4}, not just 4. IMO, it should be just 4, as it has unambigious meaning, and doesn't need to be represented in value form.

No, that's not true anymore. When we moved all the magic around native types to fromRDF/toRDF we changed the behavior to also allow type coercion of native types, i.e., also a JSON number will expand to a value object with a datatype if it is found in a type-coerced term. I will add a test to document this.

lanthaler added a commit that referenced this issue Oct 22, 2012
@msporny
Copy link
Member

msporny commented Nov 20, 2012

@gkellogg @dlongley @lanthaler Where are we on this issue? It seems as if it is resolved, but I don't see how it was resolved (other than adding a test underscoring that the current spec text is correct). Can we close this issue? Is the addition of the test that Markus' added enough?

@msporny
Copy link
Member

msporny commented Nov 20, 2012

We need to update all of the algorithms to take things like language maps and property generators into account. The value compaction algorithm assumes too much about the input and needs to be updated to take things like type coercion, language maps, and property generators into account.

@dlongley
Copy link
Member

One of the things to remember about the algorithm is that it is simplified because of assumptions made by term selection. If a particular term cannot be selected due to the term ranking algorithm, then we don't have to worry about losing information/conflating plain vs. typed literals when simplifying @value. This is why we can avoid having to consider @type coercion information here -- it has already been considered by the term ranking algorithm. The same likely applies (or we'd like it to apply to keep things simple at this step) to language maps and property generators.

lanthaler added a commit that referenced this issue Dec 20, 2012
lanthaler added a commit that referenced this issue Dec 20, 2012
@lanthaler
Copy link
Member

I've just updated the Value Compaction algorithm to take everything into account we changed since the last update (language maps, annotations, etc.). I will therefore close this issue in 24 hours unless I hear objections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants