spaceandtimelabs / sxt-proof-of-sql

Space and Time | Proof of SQL
Other
1.58k stars 70 forks source link

Isolate `arrow` code #237

Open JayWhite2357 opened 1 week ago

JayWhite2357 commented 1 week ago

Background and Motivation

Currently, arrow is one of the elements that must be disabled to enable no_std support. Additionally, not every use-case requires arrow. So, we have gated it behind a features flag (feature = "arrow"). However, there are many instances of #[cfg(feature = "arrow")] throughout the codebase, which leave the code cluttered.

Changes Required

JayWhite2357 commented 1 week ago

@stuarttimwhite @tlovell-sxt Is 53 the correct version for arrow?

JayWhite2357 commented 1 week ago

/bounty $100

algora-pbc[bot] commented 1 week ago

💎 $100 bounty • Space and Time

Steps to solve:

  1. Start working: (Optional) Comment /attempt #237 with your implementation plan. Note: we will only assign an issue if you include an implementation plan with a time estimate. Additionally, to be assigned an issue, you must have previously contributed to the project. You can still work on an issue and submit a PR without being assigned.
  2. Submit work: Create a pull request including /claim #237 in the PR body to claim the bounty
  3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

Thank you for contributing to spaceandtimelabs/sxt-proof-of-sql!

Add a bounty • Share on socials

Attempt Started (GMT+0) Solution
🟢 @melsonic Oct 12, 2024, 1:13:39 PM #267
🟢 @varshith257 Oct 12, 2024, 1:52:33 PM #266
stuarttimwhite commented 1 week ago

@stuarttimwhite @tlovell-sxt Is 53 the correct version for arrow?

Yes.

melsonic commented 6 days ago

/attempt #237

Algora profile Completed bounties Tech Active attempts Options
@melsonic 5 bounties from 4 projects
TypeScript, JavaScript,
Rust & more
Cancel attempt
melsonic commented 6 days ago

@JayWhite2357 Hey, I have gone through the codebase and have some doubts.

I see some functions inside struct implementation have functions using the arrow feature. If we try to isolate only those functions under the custom arrow module, it will create cyclic dependencies. So, is moving the whole implementation to the custom arrow module ok?

varshith257 commented 6 days ago

@melsonic I forgot to mark my attempt, I have already worked on it(left with a few fixes) and successfully isolated it to the custom arrow module. Commented for we don't make duplicate efforts for it :)

@JayWhite2357 I will submit multiple PRs to make the review process smooth. With this /attempt #237

Algora profile Completed bounties Tech Active attempts Options
@varshith257 15 bounties from 7 projects
Go, Scala,
TypeScript & more
﹟232
Cancel attempt
algora-pbc[bot] commented 6 days ago

💡 @melsonic submitted a pull request that claims the bounty. You can visit your bounty board to reward.

melsonic commented 6 days ago

Hey @varshith257, I've also spent some time going through the codebase, and So, I have submitted a PR with my changes. The code is working fine, I have tested it, and only the documentation part is left, which I will do in the next commit. Since you have also worked on the issue, I am open to collaborating before the final changes if you are ok with it, else let Jay decide.

varshith257 commented 6 days ago

@melsonic Cool! Let's collaborate. Yes, we have both spent time on it. I think we have two different solutions, so let's see what @JayWhite2357 agree with and let's collaborate on it

algora-pbc[bot] commented 6 days ago

💡 @varshith257 submitted a pull request that claims the bounty. You can visit your bounty board to reward.

melsonic commented 5 days ago

@melsonic Cool! Let's collaborate. Yes, we have both spent time on it. I think we have two different solutions, so let's see what @JayWhite2357 agree with and let's collaborate on it

Sure bro! Let's colab. And talking about the solution, it is not that different, the difference is in the structure. I also started with the module arrow inside base, but after seeing some modules under sql/proof, I moved it outside base.

And yeah, let's wait for @JayWhite2357 and then proceed with our changes.