Copilot commented on code in PR #1391: URL: https://github.com/apache/datafusion-python/pull/1391#discussion_r3000850103
########## examples/datafusion-ffi-example/python/tests/_test_config.py: ########## @@ -0,0 +1,35 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from datafusion import SessionConfig, SessionContext +from datafusion_ffi_example import MyConfig + + +def test_catalog_provider(): + config = MyConfig() + config = SessionConfig( + {"datafusion.catalog.information_schema": "true"} + ).with_extension(config) + config.set("my_config.baz_count", "42") + ctx = SessionContext(config) + + result = ctx.sql("SHOW my_config.baz_count;").collect() + assert result[0][1][0].as_py() == "42" + + ctx.sql("SET my_config.baz_count=1;") Review Comment: `SET my_config.baz_count=1;` is never executed because `SessionContext.sql()` returns a DataFrame and statements only run when an action (e.g. `.collect()`) is invoked. This makes the subsequent `SHOW` likely still return the previous value. ```suggestion ctx.sql("SET my_config.baz_count=1;").collect() ``` ########## crates/core/src/context.rs: ########## @@ -185,6 +186,28 @@ impl PySessionConfig { fn set(&self, key: &str, value: &str) -> Self { Self::from(self.config.clone().set_str(key, value)) } + + pub fn with_extension(&self, extension: Bound<PyAny>) -> PyResult<Self> { + let capsule = extension.call_method0("__datafusion_extension_options__")?; + let capsule = capsule.cast::<PyCapsule>().map_err(py_datafusion_err)?; Review Comment: Casting to `PyCapsule` uses `map_err(py_datafusion_err)?`, which wraps an existing `PyErr` into a generic `PyRuntimeError` string and loses the original exception type/context. Prefer propagating the original error (or raising a targeted `TypeError`/`ValueError` with a clear message) so users get actionable feedback when a non-capsule is returned. ```suggestion let capsule = capsule.cast::<PyCapsule>()?; ``` ########## crates/core/src/context.rs: ########## @@ -185,6 +186,28 @@ impl PySessionConfig { fn set(&self, key: &str, value: &str) -> Self { Self::from(self.config.clone().set_str(key, value)) } + + pub fn with_extension(&self, extension: Bound<PyAny>) -> PyResult<Self> { Review Comment: `with_extension` unconditionally calls `__datafusion_extension_options__` on the provided object. For consistency with other FFI entry points in this file (which check `hasattr` before calling special methods), consider validating the attribute first and raising a clear error if it’s missing so users don’t see a bare `AttributeError`. ```suggestion pub fn with_extension(&self, extension: Bound<PyAny>) -> PyResult<Self> { if !extension.hasattr("__datafusion_extension_options__")? { return Err(pyo3::exceptions::PyAttributeError::new_err( "Expected extension object to define __datafusion_extension_options__()", )); } ``` ########## examples/datafusion-ffi-example/python/tests/_test_config.py: ########## @@ -0,0 +1,35 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from datafusion import SessionConfig, SessionContext +from datafusion_ffi_example import MyConfig + + +def test_catalog_provider(): Review Comment: Test name `test_catalog_provider` doesn’t match what’s being exercised (config extension + SHOW/SET behavior). Renaming it to reflect config extension behavior will make failures easier to diagnose and keep the suite consistent. ```suggestion def test_config_extension_show_set(): ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
