Add Wrapper values #2893
Merged
Add Wrapper values #2893
Conversation
This file contains hidden or 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
zachmu
approved these changes
Mar 17, 2025
Member
zachmu
left a comment
zachmu
left a comment
There was a problem hiding this comment.
LGTM, only real comment is about perf implications, which I assume you'll be measuring in Dolt
sql/expression/comparison.go
Outdated
| return 0, ErrNilOperand.New() | ||
| } | ||
|
|
||
| if wrapperLeft, isWrapperLeft := left.(sql.AnyWrapper); isWrapperLeft { |
Member
There was a problem hiding this comment.
Have we profiled this? Type casts to an interface can be relatively expensive, not sure where this is on the hot path but I would expect it to leave a dent
Contributor
Author
There was a problem hiding this comment.
After thinking about it, there's probably not much benefit to this "comparison short-circuit", since (for address types and toast types at least) can't be used to conclude that two values are different, only that they're the same. I'll remove it.
We still need to unwrap values before passing them totypes.TypesEqual. I'll monitor how that affects Dolt performance before I submit.
| "context" | ||
| ) | ||
|
|
||
| // This file introduces interfaces for encapsulating values that may be expensive to load or deserialize. |
…firm it has a real performance benefit, and not a performance penalty.
zachmu
approved these changes
Mar 28, 2025
nicktobey
force-pushed
the
nicktobey/wrapper
branch
from
3bf5e3c to
56c0371
Compare
nicktobey
force-pushed
the
nicktobey/wrapper
branch
from
fa07a76 to
e5862b5
Compare
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.
This PR adds support for
Wrappervalues, a new type of value that the storage layer can provide to the engine. This is a generalization of the existingJsonWrapperinterface, but is not limited to JSON values.Wrapper types are useful because they can represent a value in storage without needing to fully deserialize that value for the engine. The primary use case is for "out-of-band" storage values, such as large
BLOBorTEXTvalues that aren't stored directly in tables.The Wrapper interface has the following methods:
Unwrap/UnwrapAny- These methods both deserialize the wrapped value and return it.UnwrapAnyreturns aninterface{}, whileUnwraphas a parameterized return type. The implementation is encouraged to cache this value.IsExactLength- Returns true if the size of the value can be known without deserializing it.MaxByteLength- The length of the value in bytes. IfIsExactLengthis true, this is the exact length of the value. Otherwise, it's an upper bound on the length of the value, determined by the schema of the table the value was read from.Compare- Some Wrapper implementations may be able to compare wrapped values without needing to deserialize them. This method returns a boolean indicating whether or not this "short-circuit" comparison is possible, and an int indicating the result of the comparison.