Skip to content

Conversation

@iPeluwa
Copy link
Contributor

@iPeluwa iPeluwa commented Mar 27, 2025

No description provided.

@sunng87
Copy link
Member

sunng87 commented Mar 28, 2025

@iPeluwa Thank you for the patch! The features added into this library look good to me. There are some format and lint issues.

@iPeluwa iPeluwa requested a review from sunng87 March 29, 2025 16:21
@sunng87
Copy link
Member

sunng87 commented Mar 31, 2025

@iPeluwa you can run cargo clippy and cargo fmt locally to fix those lint and format issues

Copy link
Member

@sunng87 sunng87 left a comment

Choose a reason for hiding this comment

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

Again, please configure your editor to run format on save

}
}
}
*sc_guard = new_context;
Copy link
Member

Choose a reason for hiding this comment

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

We already got this config mutable, why we need to recreate the whole session_context here?

// Build a ListArray containing "public"
let mut list_builder = ListBuilder::new(StringBuilder::new());
for _ in 0..num_rows {
list_builder.values().append_value("public");
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid hardcode public here? Is it possible to read schemas from SessionContext?

@iPeluwa iPeluwa requested a review from sunng87 April 9, 2025 16:12
@monoscope-tech monoscope-tech closed this by deleting the head repository Apr 9, 2025
@sunng87
Copy link
Member

sunng87 commented Apr 9, 2025

@iPeluwa any reason this patch is closed? Some of the content has been quite helpful to this library. If you agreed, I will pick up this by myself.

@iPeluwa
Copy link
Contributor Author

iPeluwa commented Apr 9, 2025

I'm going to open a new PR, I decided it was best to Organize things much better and then reopen a PR.

@iPeluwa any reason this patch is closed? Some of the content has been quite helpful to this library. If you agreed, I will pick up this by myself.

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.

3 participants