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: Add concatenation of LazyDataFrames #233

Merged
merged 11 commits into from
Jun 24, 2024

Conversation

davidgovea
Copy link
Contributor

Resolves #232

Exposes the underlying lazy-compatible concat, concat_lf_horizontal, and concat_lf_diagonal functions in native.

Tried to keep things reasonably inline with existing code. Added tests that verify the behavior 👍

@davidgovea davidgovea changed the title Enable concatenation of LazyDataFrames feat: Add concatenation of LazyDataFrames Jun 24, 2024
@@ -166,6 +166,7 @@ export interface LazyDataFrame extends Serialize, GroupByOps<LazyGroupBy> {
* The `fetch` operation will truly load the first `n`rows lazily.
*/
head(length?: number): LazyDataFrame;
inner(): any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Matches pattern in Series & Dataframe

inner() {
return _s;

Comment on lines +605 to +607
get [Symbol.toStringTag]() {
return "LazyDataFrame";
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Matches existing pattern. Backs the new isLazyDataFrame utils

get [Symbol.toStringTag]() {
return "DataFrame";
},

Comment on lines +38 to +44
Some("vertical") => concat(&ldfs, union_args),
Some("horizontal") => concat_lf_horizontal(&ldfs, union_args),
Some("diagonal") => concat_lf_diagonal(
&ldfs,
UnionArgs {
diagonal: true,
..union_args
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to parse the how on the rust side, and dispatch to the appropriate functions here, rather than calling different rust functions from the JS side like DataFrame does.

Seemed a bit more concise. Also tried napi string_enums but landed here.

Could use some maintainer eyes 👀 on the args here: the diagonal option is newly added to UnionArgs, and I'm not entirely sure if it's needed/desired here.

This reverts commit bf9e227.
I guess this is needed..
Copy link
Collaborator

@universalmind303 universalmind303 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks @davidgovea

@universalmind303 universalmind303 merged commit 30aecd0 into pola-rs:main Jun 24, 2024
9 checks passed
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.

Support LazyDataFrame concatenation (in node bindings)
2 participants