Add built-in functions#781
Conversation
mmarx
left a comment
There was a problem hiding this comment.
I've not yet reviewed everything, but I don't think this is ready to merge without a major overhaul. There's several issues both with respect to code quality and with respect to correctness:
- we're pulling in
chronofor handling timestamps, but then re-implement the timestamp parsing ourselves – why? - I'm not impressed with the code quality – there's functions that are defined identically in multiple files (and then inlined in other places as well), one-line functions that are only called once, redundant function calls like
Some(value.to_plain_string_unchecked())which could be a simplevalue.to_plain_string()instead, … - the hash functions are implemented for plain strings and language tagged strings, yet the spec says that they take plain strings and simple literals
- the nondeterministic functions take an arbitrary number of arguments, even though they really should not take any arguments at all
| fn int_type_propagation() -> FunctionTypePropagation { | ||
| FunctionTypePropagation::KnownOutput(StorageTypeName::Int64.bitset()) | ||
| } | ||
|
|
||
| fn double_type_propagation() -> FunctionTypePropagation { | ||
| FunctionTypePropagation::KnownOutput(StorageTypeName::Double.bitset()) | ||
| } | ||
|
|
||
| fn string_type_propagation() -> FunctionTypePropagation { | ||
| FunctionTypePropagation::KnownOutput( | ||
| StorageTypeName::Id32 | ||
| .bitset() | ||
| .union(StorageTypeName::Id64.bitset()), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Why are these separate functions? double_type_propagation is even only called once.
| if parameter_first.value_domain() != ValueDomain::PlainString { | ||
| return None; | ||
| } | ||
| if parameter_second.value_domain() != ValueDomain::Iri { | ||
| return None; | ||
| } | ||
|
|
||
| let lexical = parameter_first.to_plain_string_unchecked(); | ||
| let datatype = parameter_second.to_iri_unchecked(); | ||
|
|
||
| AnyDataValue::new_from_typed_literal(lexical, datatype).ok() |
There was a problem hiding this comment.
| if parameter_first.value_domain() != ValueDomain::PlainString { | |
| return None; | |
| } | |
| if parameter_second.value_domain() != ValueDomain::Iri { | |
| return None; | |
| } | |
| let lexical = parameter_first.to_plain_string_unchecked(); | |
| let datatype = parameter_second.to_iri_unchecked(); | |
| AnyDataValue::new_from_typed_literal(lexical, datatype).ok() | |
| AnyDataValue::new_from_typed_literal( | |
| parameter_first.to_plain_string()?, | |
| parameter_second.to_iri()? | |
| ).ok() |
|
Since "simple literals" came up in the discussion: These should not be implemented as a separate type but identified with xsd:string. This is a relic from the depth of RDF history that is vanishing as specs get updated. |
|
|
||
| let regex_str = if parameters.len() == 3 { | ||
| let flags = LangTaggedString::try_from(parameters[2].clone()).ok()?; | ||
| format!("(?{}){}", flags.string, lang_pattern.string) |
There was a problem hiding this comment.
This has, in principle, similar problems to the REPLACE implementation. It's somewhat mitigated by the regex crate not implementing backreferences – which SPARQL requires, so that's a different kind of problems. We probably want to use fancy-regex instead.
There was a problem hiding this comment.
Ah so the regex crate doesnt implement those, because they are hard to implement efficiently. But I guess we don't mind that.
There was a problem hiding this comment.
Depends on what we want – if we want to go for SPARQL compatibility, a different regex implementation is required (fancy-regex uses a hybrid approach, where the efficient NFA-based implementation is used if possible, so backreferences, lookaround, etc. become pay-as-you-go). In any case, we still shouldn't just inject flags into the pattern here, certainly not without validating them first. It's also better to use the RegexBuilder to set the options instead of splicing them into the pattern.
There was a problem hiding this comment.
Oxigraph does this correctly: https://github.com/oxigraph/oxigraph/blob/5a9a8cd81f9afe5d64187aa9912705cfade833d0/lib/spareval/src/expression.rs#L1777
| /// An optional third parameter may provide regex flags (e.g. `"i"` for case-insensitive), | ||
| /// corresponding to the SPARQL `regex(string, pattern [, flags])` function. |
There was a problem hiding this comment.
This also doesn't validate the flags, and furthermore doesn't support the q flag.
| /// Returns a language tagged string if the first parameter has a language tag. | ||
| /// Otherwise, return a plain string. |
There was a problem hiding this comment.
It would be nice if we could keep the grammar consistent and not use returns and return in the same paragraph.
| // With flags: case-insensitive match | ||
| let string_flags = AnyDataValue::new_plain_string("Hello".to_string()); | ||
| let pattern_flags = AnyDataValue::new_plain_string("hello".to_string()); | ||
| let flags = AnyDataValue::new_plain_string("i".to_string()); | ||
| let result_flags = AnyDataValue::new_boolean(true); | ||
| let actual_result_flags = StringRegex.evaluate(&[string_flags, pattern_flags, flags]); | ||
| assert!(actual_result_flags.is_some()); | ||
| assert_eq!(result_flags, actual_result_flags.unwrap()); | ||
| } |
There was a problem hiding this comment.
i is the most boring flag to test here. What we should have test for are the q flag (since that requires special handling), though that should rather be an integration test, backreferences in pattern (again an integration test), and that invalid flags lead to None.
| if *parameter_count == 0 { | ||
| current_height += 1; | ||
| } else { | ||
| current_height -= parameter_count - 1; | ||
| } |
There was a problem hiding this comment.
This is now a more complicated way to write the same thing as before, why?
| if *parameter_count == 0 { | |
| current_height += 1; | |
| } else { | |
| current_height -= parameter_count - 1; | |
| } | |
| current_height -= parameter_count - 1; |
There was a problem hiding this comment.
parameter count is a usize
There was a problem hiding this comment.
True. It's still the most(?) convoluted way to write
| if *parameter_count == 0 { | |
| current_height += 1; | |
| } else { | |
| current_height -= parameter_count - 1; | |
| } | |
| current_height -= parameter_count; | |
| current_height += 1; |
There was a problem hiding this comment.
(alternatively, make those isize here)
|
We really need to push the nondeterministic functions into SPARQL queries. Consider the following program: @import nemo :- sparql{ endpoint = <https://query.wikidata.org/sparql>, query = "SELECT ?x ?y WHERE { BIND(?x AS ?y) }"} .
@import sparql :- sparql{ endpoint = <https://query.wikidata.org/sparql>, query = "SELECT ?x WHERE { BIND(RAND() AS ?x) }"} .
foo(?x) :- nemo(RAND(), ?x) .
bar(?x) :- sparql(?x) .
|
| pub struct FuncUuid; | ||
| impl NullaryFunction for FuncUuid { | ||
| fn evaluate(&self) -> Option<AnyDataValue> { | ||
| let iri = format!("urn:uuid:{}", Uuid::new_v4()); |
There was a problem hiding this comment.
It's probably better to use v7 UUIDs here: a v7 UUID is always lexicographically greater than all the v7 UUIDs generated before it, since they include the current time in addition to randomness. SPARQL leaves the version implementation-defined: “The variant and version of the UUID is implementation dependent.”
| pub struct FuncStruuid; | ||
| impl NullaryFunction for FuncStruuid { | ||
| fn evaluate(&self) -> Option<AnyDataValue> { | ||
| Some(AnyDataValue::new_plain_string(Uuid::new_v4().to_string())) |
There was a problem hiding this comment.
see above, v7 might be more appropriate.
This PR adds new built-in functions. Closes #642 (I moved aggregate functions to a separate issue #782) and closes #769.
String functions
REPLACE(str, pattern, replacement[, flags]): regex-based string replacementlangMatches(tag, range): language tag range matching per SPARQL/RFC 4647STRTRIM(str): remove leading and trailing whitespaceSTRTRIMSTART(str): remove leading whitespace onlySTRTRIMEND(str): remove trailing whitespace onlySTRDT(lex, datatype): construct a typed literal from a lexical value and datatype IRIDate/time functions
YEAR,MONTH,DAY,HOURS,MINUTES,SECONDS: component extraction fromxsd:dateTime,xsd:date, andxsd:timevaluesTIMEZONE: timezone asxsd:dayTimeDurationTZ: timezone as a plain stringNondeterministic functions
RAND(): pseudo-random double in [0, 1)UUID(): fresh IRI-form UUIDSTRUUID(): fresh string-form UUIDNOW(): current date/time as xsd:dateTimeChanges to existing functions
REGEXnow accepts flags as an optional third parameter