Copilot commented on code in PR #214:
URL: https://github.com/apache/sedona-db/pull/214#discussion_r2425082841
##########
r/sedonadb/tests/testthat/test-dataframe.R:
##########
@@ -53,6 +53,16 @@ test_that("dataframe can be created from nanoarrow objects",
{
expect_identical(sd_collect(df, ptype = r_df), r_df)
})
+test_that("dataframe can be created from an FFI table provider", {
+ df <- as_sedonadb_dataframe(data.frame(one = 1, two = "two"))
+ provider <-df$df$to_provider()
Review Comment:
Missing space after assignment operator. Should be `provider <-
df$df$to_provider()`.
##########
r/sedonadb/src/rust/src/dataframe.rs:
##########
@@ -83,12 +85,22 @@ impl InternalDataFrame {
Ok(())
}
- fn to_arrow_stream(&self, out: savvy::Sexp) -> Result<()> {
+ fn to_arrow_stream(&self, out: savvy::Sexp, requested_schema_xptr:
savvy::Sexp) -> Result<()> {
let out_void = unsafe { savvy_ffi::R_ExternalPtrAddr(out.0) };
if out_void.is_null() {
return Err(savvy_err!("external pointer to null in
to_arrow_stream()"));
}
+ let maybe_requested_schema = if requested_schema_xptr.is_null() {
+ None
+ } else {
+ Some(import_schema(requested_schema_xptr))
+ };
+
+ if maybe_requested_schema.is_some() {
Review Comment:
The code imports a schema but then immediately errors if it exists. This
creates unnecessary work and is confusing. Consider checking if the schema is
null before attempting to import it.
```suggestion
if !requested_schema_xptr.is_null() {
```
##########
r/sedonadb/src/init.c:
##########
@@ -16,9 +16,9 @@
// under the License.
#include <Rinternals.h>
+#include <stdint.h>
Review Comment:
[nitpick] The include order has been changed to put `#include <stdint.h>`
before `#include <R_ext/Parse.h>`. Consider maintaining consistent include
ordering, typically with system headers first, then R headers.
```suggestion
#include <stdint.h>
#include <Rinternals.h>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
