weld-project / weld

High-performance runtime for data analytics applications
https://www.weld.rs
BSD 3-Clause "New" or "Revised" License
2.99k stars 258 forks source link

Struct builders #464

Open segeljakt opened 5 years ago

segeljakt commented 5 years ago

The docs say:

Any struct whose fields are builders can also be used as a builder. This is used to build multiple results at the same time.

But, when I compile:

|source: vec[i32]|
    let final = for(source, {appender[i32], appender[i32]}, |sink, index, element|
        merge(sink, {element, element})
    );
    {result(final.$0), result(final.$1)}

I get:

[info] 15:49:09.591: Started weld_module_compile
[info] 15:49:09.594: Started parsing program
[info] 15:49:09.608: Done parsing program
[info] 15:49:09.608: Started compiling program
[debug] 15:49:09.611: After macro substitution:
|source:vec[i32]|
  (let final:?=(for(
    source:?,
    {appender[i32],appender[i32]},
    |sink:?,index:?,element:?|
      merge(sink:?,{element:?,element:?})
  ));{result(
    final.$0
  ),result(
    final.$1
  )})

[info] 15:49:09.612: Done compiling program
REPL: Compile error: Internal error: Merge called on non-builder
>>>

My expectation was that you could merge structs which only contain builders, and also call result on structs which only contain builders.

sppalkia commented 5 years ago

Try changing your merge to be {merge(sink.$0, element), merge(sink.$1, element)} (you need to explicitly specify the merge for each builder in the struct).

On Fri, Jul 19, 2019 at 6:50 AM segeljakt notifications@github.com wrote:

The docs say:

Any struct whose fields are builders can also be used as a builder. This is used to build multiple results at the same time.

But, when I compile:

|source: vec[i32]| let final = for(source, {appender[i32], appender[i32]}, |sink, index, element| merge(sink, {element, element}) ); {result(final.$0), result(final.$1)}

I get:

[info] 15:49:09.591: Started weld_module_compile [info] 15:49:09.594: Started parsing program [info] 15:49:09.608: Done parsing program [info] 15:49:09.608: Started compiling program [debug] 15:49:09.611: After macro substitution: |source:vec[i32]| (let final:?=(for( source:?, {appender[i32],appender[i32]}, |sink:?,index:?,element:?| merge(sink:?,{element:?,element:?}) ));{result( final.$0 ),result( final.$1 )})

[info] 15:49:09.612: Done compiling program REPL: Compile error: Internal error: Merge called on non-builder

My expectation was that you could merge structs which only contain builders, and also call result on structs which only contain builders.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/weld-project/weld/issues/464?email_source=notifications&email_token=AAKMEY5UTCAG6ZFX3S6B5U3QAHBDJA5CNFSM4IFGFODKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HAIYPGA, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKMEY7AVMZRA2UALOSFILDQAHBDJANCNFSM4IFGFODA .

-- Shoumik

segeljakt commented 5 years ago

Ok, thanks, makes sense :)

I have another question regarding builders. What happens internally in Weld when you swap the field positions of two builders? E.g:

|source: vec[i32]|
    let final = for(source, {appender[i32], appender[i32]}, |sink, index, element|
        {merge(sink.$1, 1), merge(sink.$0, 0)}
    );
    {result(final.$0), result(final.$1)}
sppalkia commented 5 years ago

That's fine as long as you maintain their linearity property (i.e., each builder should be consumed exactly once per control path) and the types of the swapped builders are the same if you're inside a for loop. We don't have a compile time check for this at the moment unfortunately (it's in the works though!)

On Fri, Jul 19, 2019 at 8:09 AM segeljakt notifications@github.com wrote:

Ok, thanks, makes sense :)

I have another question regarding builders. What happens internally in Weld when you swap the field positions of two builders? E.g:

|source: vec[i32]| let final = for(source, {appender[i32], appender[i32]}, |sink, index, element| {merge(sink.$1, 1), merge(sink.$0, 0)} ); {result(final.$0), result(final.$1)}

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/weld-project/weld/issues/464?email_source=notifications&email_token=AAKMEY776IQO2UN2HVRVQHTQAHKLVA5CNFSM4IFGFODKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2L5DZQ#issuecomment-513266150, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKMEY6NPDYUVNSAC2266NTQAHKLVANCNFSM4IFGFODA .

-- Shoumik

sppalkia commented 5 years ago

Sorry, realized you asked what happens internally: you can think of the builder as a C struct, so swapping them within a struct will effectively swap the struct values.

On Fri, Jul 19, 2019 at 8:46 AM Shoumik Palkar shoumikpalkar@gmail.com wrote:

That's fine as long as you maintain their linearity property (i.e., each builder should be consumed exactly once per control path) and the types of the swapped builders are the same if you're inside a for loop. We don't have a compile time check for this at the moment unfortunately (it's in the works though!)

On Fri, Jul 19, 2019 at 8:09 AM segeljakt notifications@github.com wrote:

Ok, thanks, makes sense :)

I have another question regarding builders. What happens internally in Weld when you swap the field positions of two builders? E.g:

|source: vec[i32]| let final = for(source, {appender[i32], appender[i32]}, |sink, index, element| {merge(sink.$1, 1), merge(sink.$0, 0)} ); {result(final.$0), result(final.$1)}

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/weld-project/weld/issues/464?email_source=notifications&email_token=AAKMEY776IQO2UN2HVRVQHTQAHKLVA5CNFSM4IFGFODKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2L5DZQ#issuecomment-513266150, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKMEY6NPDYUVNSAC2266NTQAHKLVANCNFSM4IFGFODA .

-- Shoumik

-- Shoumik