timsaucer commented on code in PR #1253:
URL:
https://github.com/apache/datafusion-python/pull/1253#discussion_r2390911560
##########
python/tests/test_expr.py:
##########
@@ -53,6 +54,10 @@
ensure_expr_list,
)
+# Avoid passing boolean literals positionally (FBT003). Use a named constant
+# so linters don't see a bare True/False literal in a function call.
+_TRUE = True
Review Comment:
I think this reduces code readability. Instead we can whitelist functions
like `lit`
##########
python/tests/test_expr.py:
##########
@@ -53,6 +54,10 @@
ensure_expr_list,
)
+# Avoid passing boolean literals positionally (FBT003). Use a named constant
+# so linters don't see a bare True/False literal in a function call.
+_TRUE = True
Review Comment:
https://docs.astral.sh/ruff/settings/#lint_flake8-boolean-trap_extend-allowed-calls
##########
src/expr/conditional_expr.rs:
##########
@@ -15,40 +15,101 @@
// specific language governing permissions and limitations
// under the License.
-use crate::{errors::PyDataFusionResult, expr::PyExpr};
+use std::sync::Arc;
+
+use crate::{
+ errors::{PyDataFusionError, PyDataFusionResult},
+ expr::PyExpr,
+};
use datafusion::logical_expr::conditional_expressions::CaseBuilder;
+use parking_lot::{Mutex, MutexGuard};
use pyo3::prelude::*;
-#[pyclass(name = "CaseBuilder", module = "datafusion.expr", subclass)]
-pub struct PyCaseBuilder {
- pub case_builder: CaseBuilder,
+struct CaseBuilderHandle<'a> {
+ guard: MutexGuard<'a, Option<CaseBuilder>>,
+ builder: Option<CaseBuilder>,
Review Comment:
Does the `builder` in here need to be `Option<>`? From reviewing the code
below it seems like it must be `Some` unless I missed something.
##########
python/tests/test_pyclass_frozen.py:
##########
@@ -0,0 +1,76 @@
+"""Ensure exposed pyclasses default to frozen."""
+
+from __future__ import annotations
+
+import re
+from dataclasses import dataclass
+from pathlib import Path
+from typing import Iterator
+
+PYCLASS_RE = re.compile(r"#\[\s*pyclass\s*(?:\((?P<args>.*?)\))?\s*\]",
re.DOTALL)
+ARG_STRING_RE =
re.compile(r"(?P<key>[A-Za-z_][A-Za-z0-9_]*)\s*=\s*\"(?P<value>[^\"]+)\"")
+STRUCT_NAME_RE =
re.compile(r"\b(?:pub\s+)?(?:struct|enum)\s+(?P<name>[A-Za-z_][A-Za-z0-9_]*)")
+
+
+@dataclass
+class PyClass:
+ module: str
+ name: str
+ frozen: bool
+ source: Path
+
+
+def iter_pyclasses(root: Path) -> Iterator[PyClass]:
+ for path in root.rglob("*.rs"):
+ text = path.read_text(encoding="utf8")
+ for match in PYCLASS_RE.finditer(text):
+ args = match.group("args") or ""
+ frozen = re.search(r"\bfrozen\b", args) is not None
+
+ module = None
+ name = None
+ for arg_match in ARG_STRING_RE.finditer(args):
+ key = arg_match.group("key")
+ value = arg_match.group("value")
+ if key == "module":
+ module = value
+ elif key == "name":
+ name = value
+
+ remainder = text[match.end() :]
+ struct_match = STRUCT_NAME_RE.search(remainder)
+ struct_name = struct_match.group("name") if struct_match else None
+
+ yield PyClass(
+ module=module or "datafusion",
+ name=name or struct_name or "<unknown>",
+ frozen=frozen,
+ source=path,
+ )
+
+
+def test_pyclasses_are_frozen() -> None:
+ allowlist = {
+ # NOTE: Any new exceptions must include a justification comment in the
Rust source
+ # and, ideally, a follow-up issue to remove the exemption.
+ ("datafusion.common", "SqlTable"),
+ ("datafusion.common", "SqlView"),
+ ("datafusion.common", "DataTypeMap"),
+ ("datafusion.expr", "TryCast"),
+ ("datafusion.expr", "WriteOp"),
+ }
+
+ unfrozen = [
+ pyclass
+ for pyclass in iter_pyclasses(Path("src"))
+ if not pyclass.frozen and (pyclass.module, pyclass.name) not in
allowlist
+ ]
+
+ assert not unfrozen, (
+ "Found pyclasses missing `frozen`; add them to the allowlist only with
a "
+ "justification comment and follow-up plan:\n" +
+ "\n".join(
+ f"- {pyclass.module}.{pyclass.name} (defined in {pyclass.source})"
+ for pyclass in unfrozen
+ )
Review Comment:
I love these tests. Well done!
--
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]
