Skip to content
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

feat: inner join #71

Merged
merged 105 commits into from
Jul 1, 2024
Merged

feat: inner join #71

merged 105 commits into from
Jul 1, 2024

Conversation

OsamaAlkhodairy
Copy link
Contributor

@OsamaAlkhodairy OsamaAlkhodairy commented Jun 17, 2024

Straightforward implementation from the Inner Join spec: https://docs.google.com/document/d/1HudyqvMzlWKA4eT0IjkIKCbidQCH3utSXXR-9vm_xcI/edit

Also added some helper function to the Page struct that are useful in testing.

Closes INT-1532

bfan05 and others added 30 commits May 29, 2024 16:49
chips/src/inner_join/controller.rs Outdated Show resolved Hide resolved
chips/src/inner_join/controller.rs Outdated Show resolved Hide resolved
chips/src/inner_join/controller.rs Outdated Show resolved Hide resolved
chips/src/inner_join/controller.rs Show resolved Hide resolved
/// Multiplicity of idx in output_table
pub out_mult: T,
/// Indiates if this row is extra and should be ignored
pub is_extra: T,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior of is_extra is almost same to is_alloc. Can we reuse that logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reuse that logic where?

chips/src/inner_join/intersector/mod.rs Show resolved Hide resolved
impl MyFinalTableAir {
#[allow(clippy::too_many_arguments)]
pub fn new(
t1_output_bus_index: usize,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reuse the parameter struct I mentioned in controller constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we only use 3 bus indices

chips/src/inner_join/controller.rs Outdated Show resolved Hide resolved

#[allow(dead_code)]
struct TableCommitments<SC: StarkGenericConfig> {
t1_commitment: Com<SC>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit hard to know what t1/t2 refers. Can we call its formal names? From chatgpt:

  1. Parent Table (or Referenced Table): This is the table that contains the primary key, which is referenced by the foreign key in the other table. It is often referred to as the "parent" because it holds the original values that are being referenced.

  2. Child Table (or Referencing Table): This is the table that contains the foreign key, which references the primary key in the parent table. It is often called the "child" because it depends on the parent table for its foreign key values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you only the the commitments struct or everywhere?

I think it's annoying and verbose to use those names everywhere. For example, I think parent_table_intersector_bus_index becomes too much. I added a comment above the constructor to note which table is T1 and which is T2.

where
Val<SC>: Field,
{
Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assert validity of
fkey_start: usize,
fkey_end: usize,
t1_idx_len: usize,
t1_data_len: usize,
t2_idx_len: usize,
t2_data_len: usize,
idx_limb_bits: usize,
decomp: usize,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is there to validate for idx_len, data_len, limb_bits, decomp?

Copy link

linear bot commented Jun 20, 2024

@jonathanpwang
Copy link
Contributor

@nyunyunyunyu can you take a look at this again

Copy link
Contributor

@nyunyunyunyu nyunyunyunyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Feel free to merge after addressing comments

chips/src/inner_join/controller.rs Outdated Show resolved Hide resolved
chips/src/inner_join/final_table/mod.rs Outdated Show resolved Hide resolved
chips/src/inner_join/intersector/mod.rs Show resolved Hide resolved
@OsamaAlkhodairy OsamaAlkhodairy merged commit 3132afc into main Jul 1, 2024
3 checks passed
@OsamaAlkhodairy OsamaAlkhodairy deleted the feat/inner_join branch July 1, 2024 16:22
luffykai pushed a commit that referenced this pull request Dec 13, 2024
* feat: sorted_limbs chip checking each limb less than limb_bits bits

* wip

* feat: completed sorted_limbs chip with tests

* wip

* feat: SortedLimbsChip with LessThan subchip

* feat: less_than subchip refactored

* feat: rename SortedLimbsChip to AssertSortedChip and write LessThanChip tests

* chore: change name of assert sorted chip

* chore: fix names in tests for AssertSortedChip

* chore: address comments

* chore: cleanup

* chore: change MAX from generic to instance field for LessThanChip and AssertSortedChip

* wip: added extra bits to middle chip trace

* wip

* adding connection to IsEqualVec chip

* feat: IsLessThanChip to compare two numbers

* feat: IsLessThanTuple subchip for different limb_bits

* test: added tests for partially and non-allocated pages

* test: added negative tests

* added constraints

* adding comments

* feat: IsLessThanTupleChip subchip in AssertSortedChip

* renaming

* removing TODO comments

* fixing clippy

* chore: renaming to idx and data

* chore: address comments first pass

* chore: moving page_controller inside page_read

* chore: refactor AssertSorted, IsEqual, IsLessThan, and IsLessThanTuple chips

* chore: address comments

* chore: eliminate high dim poly from IsLessThanTupleChip

* chore: fix tests

* chore: address comments for AssertSortedChip

* chore: cleanup AssertSorted

* chore: cleanup

* chore: include roundtrip flatten and from_slice tests

* feat: flatten and from_slice for IO and Aux columns

* wip: final page chip

* remove txt file

* feat: final_page_chip

* feat: integrating all chips for page_read_write

* fixing lib

* using field bits()

* adding extra communication between checker and final chip

* muting clippy

* renaming to AIR

* adding comments

* comment fix

* optimization to one less bus

* comments

* moving old page_controller inside page_read

* updateing comment

* wip: inner join

* feat: more general FinalPageAir

* making IsEqualVecAuxCols clonable

* enforcing unallocated rows to be zero

* aligning with new less than chip

* wip

* using Air::eval instead of SubAir::eval

* fix comment

* wip

* removed arguments from load_page for readability

* addressing comments and adding common/ directory with Page struct

* wip

* controller complete

* controller complete

* adding less than constraints

* adding comments

* more comments

* added keygen and prove functions to controller

* added verify function to controller

* addressing comments

* merging new chanegs

* some renaming

* addressing comments

* removing too_many_arguments bypassing

---------

Co-authored-by: bfan <76703988+bfan05@users.noreply.github.com>
Co-authored-by: Jonathan Wang <31040440+jonathanpwang@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants