-
Notifications
You must be signed in to change notification settings - Fork 184
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
Add FixedSizeBinary
with support up to at least 32 bytes.
#229
Comments
/bounty $1000 |
💎 $1,000 bounty • Space and TimeSteps to solve:
Thank you for contributing to spaceandtimelabs/sxt-proof-of-sql! Add a bounty • Share on socials
|
/attempt #229
|
Hello @JayWhite2357 I’d be happy to take on this issue. I think it could be broken down into multiple PRs for smaller, easier-to-test patches. Let me know if that works for you, or if you’d prefer to handle it all in one go. I’ve just joined the Discord and would be happy to discuss it further. |
@tareknaser I agree that this makes sense to be broken into smaller PRs. We much prefer smaller PRs over large PRs. |
It looks like there are multiple people working on this issue. It may be wise for you to coordinate via Discord. |
It’s a bit tricky to break the issue into clear pieces right now. I think it’ll become clearer once we begin the actual implementation. I can start with a small PR to implement Would that be okay with you, @Ashu999 ? |
sure, go ahead @tareknaser |
Testing out two other bounty program options here: |
Total Bounty: $1000 BountiesBounties requiring verification after close:
|
Hi @JayWhite2357 - Is this issue still available to pick up? |
Background and Motivation
Many data are binary blobs. In particular, the EVM address and hash types are 20 bytes and 32 bytes respectively. We should support these natively within Proof of SQL. See https://docs.rs/arrow/latest/arrow/datatypes/enum.DataType.html#variant.FixedSizeBinary.
Changes Requested
Add full support for a
FixedSizeBinary
type. In order to do this there are several considerations.Scalar
types support naturally embedding 31 bytes but not 32 bytes, there should be a divergence in how <=31 bytes are handled and 32 bytes are handled.Scalar
type. This means simply relying on the conversion from[u64; 4]
to theScalar
.Scalar
. This is identical to howString
s are handled. Seesxt-proof-of-sql/crates/proof-of-sql/src/base/scalar/mont_scalar_from.rs
Line 16 in 48b2f0b
ColumnType
should probably have a new variantFixedSizeBinary(i32)
added. It is not as clear what the variant forColumn
andOwnedColumn
should be.FixedSizeBinary(i32, &'a [u8])
andFixedSizeBinary(i32, Vec<u8>)
respectively, but that may not make sense. The reason for this suggestion is because something likeVec<Vec<u8>>
has a lot of overhead, andVec<[u8]>
is not allowed by Rust.Binary
instead ofFixedSizeBinary
.Scalar
conversions explicit rather than usingFrom
/Into
trait. #228 is partially motivated by this issue, and may make this issue a bit more ergonomic. However, working on them in parallel will likely lead to some branch conflict, so pay attention to that issue.=
) and inequality (!=
). This is because there is no way to add, subtract, etc. between binary data. Eventually, we may wish to add bitwise operations, but those are beyond the scope of this issue.The text was updated successfully, but these errors were encountered: