-
Notifications
You must be signed in to change notification settings - Fork 605
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(sink): support deltalake sink with rust sdk #13600
Changes from 14 commits
e6bc0ee
013eede
f0047ba
2ff021f
7b85ea2
6fbbac3
c6a709c
1ddf1f9
cdecda5
f521372
c890167
2a35a5e
0aa6785
9b7b543
e460901
13ff6ed
c187f72
2217f61
4c503fa
1aaf516
e1a3814
76547e4
b0b1b87
50899c0
10b1ba6
5a492a6
435eb73
cf96d64
4a83b8f
814dcc9
f9605ad
899070a
86625ab
5c365a9
a4023e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
pub use arrow_impl::to_record_batch_with_schema; | ||
wenym1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
use {arrow_array, arrow_buffer, arrow_cast, arrow_schema}; | ||
|
||
#[allow(clippy::duplicate_mod)] | ||
#[path = "./arrow.rs"] | ||
mod arrow_impl; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
pub use arrow_impl::to_record_batch_with_schema as to_deltalake_record_batch_with_schema; | ||
wenym1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
use { | ||
arrow_array_deltalake as arrow_array, arrow_buffer_deltalake as arrow_buffer, | ||
arrow_cast_deltalake as arrow_cast, arrow_schema_deltalake as arrow_schema, | ||
}; | ||
|
||
#[allow(clippy::duplicate_mod)] | ||
#[path = "./arrow.rs"] | ||
mod arrow_impl; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, this is quite interesting! 😮 How did you come up with this? Any other projects used this trick?
So my thoughts:
Nits:
Changes requested:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's generally LGTM. Might also hear others' opinion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Initially in c890167 I use macro too generate the same code for different version with the same trick, which set different arrow version to the same arrow mod name for different conversion code implementation. But I think this is too hacky and the IDE will fail to index a large part of code. And then I looked for approach to include the code within an outer file to a file so that I don't have to include the code in a long macro, and I found that the current approach is a perfect match😄 And I don't see any other project using this trick yet.
Currently arrow does not flow in our internal system. It's only used to interact with external systems, so in short term I don't think there will be feature requirement on arrow48 to arrow49. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
mod arrow_common; | ||
wenym1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
mod arrow_deltalake; | ||
|
||
pub use arrow_common::to_record_batch_with_schema; | ||
pub use arrow_deltalake::to_deltalake_record_batch_with_schema; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason for downgrading arrow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because both Delta.rs and IceLake need to use Arrow, version 48.0.1 is the maximum version they both support.