Fix index read error #583
Merged
Fix index read error #583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Upon transaction, flakes with time types (`xsd:time`, `xsd:date`, `xsd:dateTime`) have their values coerced to `java.time`/`js/Date` objects. When writing index files to disk, we need to write out readable strings for these objects, and then re-coerce them into time objects upon load. Previously we were writing files with unreadable tagged literals in them. Note: This writes out time strings with maximal precision, allowing us to reuse our existing datatype coercion code when loading (time strings with trailing zeros trimmed do not always satisfy our regexes). This makes for a simpler solution, but it does mean potential wasted space in our index files. We can likely optimize this in the future by having more clever formatters and/or having deserialization-specific coercion logic that recognizes truncate d values.
Fixes #580 The built-in java parsers will do the correct thing for subsecond values. We don't need to be doing any manual string-inspecting apart from determining whether an offset is present.
Previously, this value was being written as an `xsd:dateTime`, but that is not a valid `xsd:dateTime` value: https://books.xmlschemata.org/relaxng/ch19-77049.html Apart from just being incorrect, this was causing this one flake to be an anomaly among the `xsd:DateTime` flakes, breaking the assumptions needed for serialization/deserialization.
…d:dateTime` flakes Now that the `commit:time` flake is no longer erroneously of type `xsd:dateTime`, any time-typed flakes we encounter should be standardized to time objects. Therefore, we don't need to check the type, and can just use the formatters directly.
|
In the course of working on this, I discovered we have the same issue with commit files. I made a separate issue for it here: #584 Once this PR is accepted/merged, then the fix for that one can build off of it. |
dpetran
reviewed
Sep 29, 2023
|
|
||
| (if-let [offset (peek matches)] | ||
| (OffsetDateTime/parse s) | ||
| (LocalDateTime/parse s)) |
There was a problem hiding this comment.
This is certainly a lot simpler! 😃
dpetran
approved these changes
Sep 29, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes https://github.com/fluree/core/issues/31
Fixes #580
When creating flakes with time-typed values (
xsd:dateTime,xsd:date,xsd:time), we coerce the values intojava.time/js/Dateobjects. This index-reading bug was caused by not serializing those values correctly, resulting in unreadable index files. This PR introduces special serialization/deserialization logic for these types .We were writing out flakes with unreadable tagged literals, eg:
Which was causing parsing errors:
Custom formatters are used during serialization to ensure that our existing
datatype/coercecode can be reused to deserialize these values upon load. This is because the defaultstr/.toString()behavior will trim off trailing zeros, which produces values that are not valid for the given datatype. This is a simple initial implementation, we can potentially optimize in the future with more clever formatters and/or a separate coercion path for deserialization.For #580, I switched coercion for
xsd:timeandxsd:dateTimein clj to use the java parsing functions directly. Our hard-coded multiplication of nanoseconds was causing errors/incorrect values, whereas the built-in parsing fns will do the correct thing with those values. I'm not sure if there was a reason to not use these parsing fns in the first place, if there's a significant downside I wasn't aware of then we should discuss other alternatives.