mbrobbel commented on code in PR #2279:
URL: https://github.com/apache/arrow-adbc/pull/2279#discussion_r1827563008
##########
rust/drivers/datafusion/src/lib.rs:
##########
@@ -223,23 +240,86 @@ impl Optionable for DataFusionConnection {
_key: Self::Option,
_value: adbc_core::options::OptionValue,
) -> adbc_core::error::Result<()> {
- todo!()
+ match _key.as_ref() {
+ constants::ADBC_CONNECTION_OPTION_CURRENT_CATALOG => match _value {
Review Comment:
You can remove the `_` prefix from these args now
(https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/builtin/static.UNUSED_VARIABLES.html#explanation).
```suggestion
match key.as_ref() {
constants::ADBC_CONNECTION_OPTION_CURRENT_CATALOG => match value
{
```
##########
rust/drivers/datafusion/src/lib.rs:
##########
@@ -223,23 +240,86 @@ impl Optionable for DataFusionConnection {
_key: Self::Option,
_value: adbc_core::options::OptionValue,
) -> adbc_core::error::Result<()> {
- todo!()
+ match _key.as_ref() {
+ constants::ADBC_CONNECTION_OPTION_CURRENT_CATALOG => match _value {
+ OptionValue::String(value) => {
+ self.runtime.block_on(async {
+ let query = format!(
+ "SET datafusion.catalog.default_catalog = {}",
+ value.as_str()
+ );
+ self.ctx.sql(query.as_str()).await.unwrap();
+ });
+ Ok(())
+ }
+ _ => Err(Error::with_message_and_status(
+ "CurrentCatalog value must be of type String",
+ Status::InvalidArguments,
+ )),
+ },
+ constants::ADBC_CONNECTION_OPTION_CURRENT_DB_SCHEMA => match
_value {
+ OptionValue::String(value) => {
+ self.runtime.block_on(async {
+ let query =
+ format!("SET datafusion.catalog.default_schema =
{}", value.as_str());
Review Comment:
```suggestion
format!("SET datafusion.catalog.default_schema =
{value}");
```
##########
rust/drivers/datafusion/src/lib.rs:
##########
@@ -707,39 +789,81 @@ pub struct DataFusionStatement {
ctx: Arc<SessionContext>,
sql_query: Option<String>,
substrait_plan: Option<Plan>,
+ bound_record_batch: RefCell<Option<RecordBatch>>,
Review Comment:
I don't think you need a `RefCell` here. Because you are moving the
`RecordBatch` (in in `bind` and out in `execute_update`), you can just use
`Option`.
```suggestion
bound_record_batch: Option<RecordBatch>,
```
##########
rust/drivers/datafusion/src/lib.rs:
##########
@@ -223,23 +240,86 @@ impl Optionable for DataFusionConnection {
_key: Self::Option,
_value: adbc_core::options::OptionValue,
) -> adbc_core::error::Result<()> {
- todo!()
+ match _key.as_ref() {
+ constants::ADBC_CONNECTION_OPTION_CURRENT_CATALOG => match _value {
+ OptionValue::String(value) => {
+ self.runtime.block_on(async {
+ let query = format!(
+ "SET datafusion.catalog.default_catalog = {}",
+ value.as_str()
Review Comment:
```suggestion
"SET datafusion.catalog.default_catalog =
{value}"
```
##########
rust/drivers/datafusion/src/lib.rs:
##########
@@ -768,13 +892,31 @@ impl Statement for DataFusionStatement {
}
fn execute_update(&mut self) -> adbc_core::error::Result<Option<i64>> {
- self.runtime.block_on(async {
- let _ = self
- .ctx
- .sql(&self.sql_query.clone().unwrap())
- .await
- .unwrap();
- });
+ if self.sql_query.is_some() {
+ self.runtime.block_on(async {
+ let _ = self
+ .ctx
+ .sql(&self.sql_query.clone().unwrap())
+ .await
+ .unwrap();
+ });
+ } else if self.bound_record_batch.get_mut().is_some() {
+ self.runtime.block_on(async {
+ let batch: RecordBatch =
self.bound_record_batch.replace(None).unwrap();
Review Comment:
Using `Option::take` instead:
```suggestion
} else if let Some(batch) = self.bound_record_batch.take() {
self.runtime.block_on(async {
```
##########
rust/drivers/datafusion/src/lib.rs:
##########
@@ -707,39 +789,81 @@ pub struct DataFusionStatement {
ctx: Arc<SessionContext>,
sql_query: Option<String>,
substrait_plan: Option<Plan>,
+ bound_record_batch: RefCell<Option<RecordBatch>>,
+ ingest_target_table: Option<String>,
}
impl Optionable for DataFusionStatement {
type Option = OptionStatement;
fn set_option(
&mut self,
- _key: Self::Option,
- _value: adbc_core::options::OptionValue,
+ key: Self::Option,
+ value: adbc_core::options::OptionValue,
) -> adbc_core::error::Result<()> {
- todo!()
+ match key.as_ref() {
+ constants::ADBC_INGEST_OPTION_TARGET_TABLE => match value {
+ OptionValue::String(value) => {
+ self.ingest_target_table = Some(value);
+ Ok(())
+ }
+ _ => Err(Error::with_message_and_status(
+ "IngestOptionTargetTable value must be of type String",
+ Status::InvalidArguments,
+ )),
+ },
+ _ => Err(Error::with_message_and_status(
+ format!("Unrecognized option: {key:?}"),
+ Status::NotFound,
+ )),
+ }
}
- fn get_option_string(&self, _key: Self::Option) ->
adbc_core::error::Result<String> {
- todo!()
+ fn get_option_string(&self, key: Self::Option) ->
adbc_core::error::Result<String> {
+ match key.as_ref() {
+ constants::ADBC_INGEST_OPTION_TARGET_TABLE => {
+ let target_table = self.ingest_target_table.clone();
+ match target_table {
+ Some(table) => Ok(table),
+ None => Err(Error::with_message_and_status(
+ format!("{} has not been set", key.as_ref()),
+ Status::NotFound,
+ )),
+ }
+ }
+ _ => Err(Error::with_message_and_status(
+ format!("Unrecognized option: {key:?}"),
+ Status::NotFound,
+ )),
+ }
}
- fn get_option_bytes(&self, _key: Self::Option) ->
adbc_core::error::Result<Vec<u8>> {
- todo!()
+ fn get_option_bytes(&self, key: Self::Option) ->
adbc_core::error::Result<Vec<u8>> {
+ Err(Error::with_message_and_status(
+ format!("Unrecognized option: {key:?}"),
+ Status::NotFound,
+ ))
}
- fn get_option_int(&self, _key: Self::Option) ->
adbc_core::error::Result<i64> {
- todo!()
+ fn get_option_int(&self, key: Self::Option) ->
adbc_core::error::Result<i64> {
+ Err(Error::with_message_and_status(
+ format!("Unrecognized option: {key:?}"),
+ Status::NotFound,
+ ))
}
- fn get_option_double(&self, _key: Self::Option) ->
adbc_core::error::Result<f64> {
- todo!()
+ fn get_option_double(&self, key: Self::Option) ->
adbc_core::error::Result<f64> {
+ Err(Error::with_message_and_status(
+ format!("Unrecognized option: {key:?}"),
+ Status::NotFound,
+ ))
}
}
impl Statement for DataFusionStatement {
- fn bind(&mut self, _batch: arrow_array::RecordBatch) ->
adbc_core::error::Result<()> {
- todo!()
+ fn bind(&mut self, batch: arrow_array::RecordBatch) ->
adbc_core::error::Result<()> {
+ self.bound_record_batch.replace(Some(batch));
Review Comment:
Using `Option::replace` instead:
```suggestion
self.bound_record_batch.replace(batch);
```
--
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]
