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

support day, month and year function for date #1154

Merged
merged 2 commits into from
Jul 3, 2021

Conversation

FlorianLudwig
Copy link
Contributor

Fixes #1153

Proposed Changes

  • Support day, month and year sparql function on date as well as datetime

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 75.595% when pulling 51a17fd on FlorianLudwig:support_date_type into 652d2e6 on RDFLib:master.

@coveralls
Copy link

coveralls commented Aug 31, 2020

Coverage Status

Coverage decreased (-0.07%) to 75.423% when pulling 7c1d49b on FlorianLudwig:support_date_type into dde9db8 on RDFLib:master.

@white-gecko white-gecko changed the title support day, nonth and year function for date support day, month and year function for date Sep 8, 2020
@white-gecko
Copy link
Member

white-gecko commented Sep 8, 2020

According to the SPARQL 1.1 Specification (https://www.w3.org/TR/sparql11-query/#func-date-time) the builtin functions are only defined on xsd:dataTime not on xsd:date. So I'm not sure if it is a good idea to extend to to xsd:date as well. If we want to extend it we might also want to extend it to xsd:gDay, xsd:gMonth, xsd:gMonthDay, xsd:gYear, and xsd:gYearMonth. While some of the builtins would result in an empty string in some cases.

@FlorianLudwig
Copy link
Contributor Author

FlorianLudwig commented Sep 8, 2020

@white-gecko I am no expert on the specs, I searched through it and could not find any specification on what to do if the argument is of the wrong type (date vs dateTime) - except for the string-functions, where it is explicitly said that it should result in an error if the argument is not a string. So this might be undefined behaviour?

If you would be interested in merging this I would look into supporting the other types as well and update this MR.

Note: xsd:date (and others) are not part of sparql 1.1 at all. Interestingly the first - and so far only - written enhancement proposal for 1.2. So I guess there seems to be interest in this from others as well :)

@white-gecko
Copy link
Member

I have seen it in different implementations that only dateTime was supported for these builtins. So it is a tough decision to make weather to support all of the extra datatype or non.
Maybe we should introduce a new switch for this behavior like:
https://rdflib.readthedocs.io/en/stable/apidocs/rdflib.plugins.sparql.html#module-rdflib.plugins.sparql

rdflib.plugins.sparql.EXTRA_DATA_TYPES = true

or

rdflib.plugins.sparql.EXTRA_DATE_DATA_TYPES = true

@FlorianLudwig
Copy link
Contributor Author

@white-gecko to be honest i have developed a dislike for switches in software as they complicate code and testing as resulting combinations do increase exponentially. So if it does not clash with the specs I prefer to not have a switch.

I would really like to simplify the code base for sparql evaluation a bit - I am running with a few local changes which gave my use case 25% performance boost - and I would like to work on it a bit more, as it is still to slow for my use case. Those where really low hanging fruit and I am sure there is more. Introducing a switch means supporting more different combinations - and probably breaking those, making it harder to move the code forward. See #1155 for an example of this - my local refactoring broke the semantic of comparing a Literal with None - keeping those would have made the code more complex for a benefit I did see (or understand).

@white-gecko
Copy link
Member

In an informative section the standard says.

3.3 Other Term Constraints

In addition to numeric types, SPARQL supports types xsd:string, xsd:boolean and xsd:dateTime (see Operand Data Types). Section Operator Mapping describes the operators and section Function Definitions the functions that can be that can be applied to RDF terms.

Based on this I can not judge if we should support the additional types or not.

Your argument that it might be supported in SPARQL 1.2 sounds good to me.

Anyhow I ask @ashleysommer and @nicholascar for their opinion.

@ashleysommer
Copy link
Contributor

I am running with a few local changes which gave my use case 25% performance boost - and I would like to work on it a bit more, as it is still to slow for my use case

That sounds great! I've had a mental note for months to go in to see where SPARQL could be made faster in RDFLib.
It is the only thing that significantly slows down execution of PySHACL. When not using any SPARQL features, the SHACL validation engine executes approx 10x faster than when SPARQL features are used.

@nicholascar
Copy link
Member

Official reviews from me and @white-gecko needed, a re-review from @ashleysommer please!

@nicholascar
Copy link
Member

I've resolved the incidental questions, since they have answers.

Based on this I can not judge if we should support the additional types or not.

I personally use xsd:date and Python equivalents instead of dateTime wherever I can to ensure we don't get spurious time values in data.

So I approve this PR even if it is extending beyond the SPARQL 1.1 spec!

@nicholascar nicholascar merged commit 538446e into RDFLib:master Jul 3, 2021
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.

sparql builtin date functions only work with datetime, not with date
5 participants