From 9d1c3f8cfef5fd9383ee5fa69e79328b5c739231 Mon Sep 17 00:00:00 2001 From: Dmitry Patsura Date: Wed, 26 Feb 2025 17:40:12 +0100 Subject: [PATCH] fix(native): Jinja - pass kwargs correctly into Python (#9276) In Python, *args and **kwargs allow functions to accept an arbitrary number of arguments. Kwarg should be passed separately when you are calling a function. It's a separate structure. Python example: @template.function def arg_named_arguments(arg1, arg2): return "arg1: " + arg1 + ", arg2: " + arg2 Jinja example: arg_named_arguments1: {{ arg_named_arguments(arg2="2 arg", arg1="1 arg") }} arg_named_arguments2: {{ arg_named_arguments(arg1="1 arg", arg2="2 arg") }} Expected output: arg_named_arguments1: "arg1: 1 arg, arg2: 2 arg" arg_named_arguments2: "arg1: 1 arg, arg2: 2 arg" --- .../cubejs-backend-native/src/cross/clrepr.rs | 25 ++++++++--- .../src/cross/clrepr_python.rs | 44 ++++++++++++------- .../cubejs-backend-native/src/cross/mod.rs | 4 +- .../src/python/cube_config.rs | 4 +- .../cubejs-backend-native/src/python/entry.rs | 6 +-- .../src/python/python_model.rs | 4 +- .../src/python/runtime.rs | 15 ++++--- .../src/template/entry.rs | 2 +- .../src/template/mj_value/python.rs | 6 ++- .../test/__snapshots__/jinja.test.ts.snap | 9 +++- .../cubejs-backend-native/test/jinja.test.ts | 4 ++ .../cubejs-backend-native/test/python.test.ts | 1 + .../test/templates/arguments-test.yml.jinja | 5 +++ .../test/templates/jinja-instance.py | 10 +++++ .../test/templates/scoped-utils.py | 10 +++++ 15 files changed, 110 insertions(+), 39 deletions(-) diff --git a/packages/cubejs-backend-native/src/cross/clrepr.rs b/packages/cubejs-backend-native/src/cross/clrepr.rs index 9c6f4d3f4a97d..9e0b17b256668 100644 --- a/packages/cubejs-backend-native/src/cross/clrepr.rs +++ b/packages/cubejs-backend-native/src/cross/clrepr.rs @@ -15,18 +15,24 @@ use std::collections::hash_map::{IntoIter, Iter, Keys}; use std::collections::HashMap; use std::sync::Arc; +#[derive(Debug, Clone)] +pub enum CLReprObjectKind { + Object, + KWargs, +} + #[derive(Clone)] -pub struct CLReprObject(pub(crate) HashMap); +pub struct CLReprObject(pub(crate) HashMap, CLReprObjectKind); impl Default for CLReprObject { fn default() -> Self { - Self::new() + Self::new(CLReprObjectKind::Object) } } impl CLReprObject { - pub fn new() -> Self { - Self(HashMap::new()) + pub fn new(kind: CLReprObjectKind) -> Self { + Self(HashMap::new(), kind) } pub fn get(&self, key: &str) -> Option<&CLRepr> { @@ -110,6 +116,15 @@ pub enum CLRepr { Null, } +impl CLRepr { + pub fn is_kwarg(&self) -> bool { + match self { + CLRepr::Object(obj) => matches!(obj.1, CLReprObjectKind::KWargs), + _ => false, + } + } +} + impl std::fmt::Display for CLRepr { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { std::fmt::Display::fmt(&self, f) @@ -179,7 +194,7 @@ impl CLRepr { Ok(CLRepr::Array(r)) } else if from.is_a::(cx) { - let mut obj = CLReprObject::new(); + let mut obj = CLReprObject::new(CLReprObjectKind::Object); let v = from.downcast_or_throw::(cx)?; let properties = v.get_own_property_names(cx)?; diff --git a/packages/cubejs-backend-native/src/cross/clrepr_python.rs b/packages/cubejs-backend-native/src/cross/clrepr_python.rs index 3a834dba55346..9712206845cc9 100644 --- a/packages/cubejs-backend-native/src/cross/clrepr_python.rs +++ b/packages/cubejs-backend-native/src/cross/clrepr_python.rs @@ -1,5 +1,5 @@ use crate::cross::clrepr::CLReprObject; -use crate::cross::{CLRepr, StringType}; +use crate::cross::{CLRepr, CLReprObjectKind, StringType}; use pyo3::exceptions::{PyNotImplementedError, PyTypeError}; use pyo3::types::{ PyBool, PyComplex, PyDate, PyDict, PyFloat, PyFrame, PyFunction, PyInt, PyList, PySequence, @@ -16,15 +16,9 @@ pub enum PythonRef { PyExternalFunction(Py), } -pub trait CLReprPython: Sized { - fn from_python_ref(v: &PyAny) -> Result; - fn into_py_impl(from: CLRepr, py: Python) -> Result; - fn into_py(self, py: Python) -> Result; -} - -impl CLReprPython for CLRepr { +impl CLRepr { /// Convert python value to CLRepr - fn from_python_ref(v: &PyAny) -> Result { + pub fn from_python_ref(v: &PyAny) -> Result { if v.is_none() { return Ok(Self::Null); } @@ -47,7 +41,7 @@ impl CLReprPython for CLRepr { Self::Int(i) } else if v.get_type().is_subclass_of::()? { let d = v.downcast::()?; - let mut obj = CLReprObject::new(); + let mut obj = CLReprObject::new(CLReprObjectKind::Object); for (k, v) in d.iter() { if k.get_type().is_subclass_of::()? { @@ -126,6 +120,16 @@ impl CLReprPython for CLRepr { }) } + fn into_py_dict_impl(obj: CLReprObject, py: Python) -> Result<&PyDict, PyErr> { + let r = PyDict::new(py); + + for (k, v) in obj.into_iter() { + r.set_item(k, Self::into_py_impl(v, py)?)?; + } + + Ok(r) + } + fn into_py_impl(from: CLRepr, py: Python) -> Result { Ok(match from { CLRepr::String(v, _) => PyString::new(py, &v).to_object(py), @@ -155,11 +159,7 @@ impl CLReprPython for CLRepr { PyTuple::new(py, elements).to_object(py) } CLRepr::Object(obj) => { - let r = PyDict::new(py); - - for (k, v) in obj.into_iter() { - r.set_item(k, Self::into_py_impl(v, py)?)?; - } + let r = Self::into_py_dict_impl(obj, py)?; r.to_object(py) } @@ -189,7 +189,19 @@ impl CLReprPython for CLRepr { }) } - fn into_py(self, py: Python) -> Result { + pub fn into_py_dict(self, py: Python) -> Result<&PyDict, PyErr> { + Ok(match self { + CLRepr::Object(obj) => Self::into_py_dict_impl(obj, py)?, + other => { + return Err(PyErr::new::(format!( + "Unable to convert {:?} into PyDict", + other.kind() + ))) + } + }) + } + + pub fn into_py(self, py: Python) -> Result { Self::into_py_impl(self, py) } } diff --git a/packages/cubejs-backend-native/src/cross/mod.rs b/packages/cubejs-backend-native/src/cross/mod.rs index 39c4ad999e003..8f9bbc4649367 100644 --- a/packages/cubejs-backend-native/src/cross/mod.rs +++ b/packages/cubejs-backend-native/src/cross/mod.rs @@ -4,7 +4,7 @@ mod clrepr_python; #[cfg(feature = "python")] mod py_in_js; -pub use clrepr::{CLRepr, CLReprKind, CLReprObject, StringType}; +pub use clrepr::{CLRepr, CLReprKind, CLReprObject, CLReprObjectKind, StringType}; #[cfg(feature = "python")] -pub use clrepr_python::{CLReprPython, PythonRef}; +pub use clrepr_python::PythonRef; diff --git a/packages/cubejs-backend-native/src/python/cube_config.rs b/packages/cubejs-backend-native/src/python/cube_config.rs index 2621ae36533b5..d612ebdc3bd4f 100644 --- a/packages/cubejs-backend-native/src/python/cube_config.rs +++ b/packages/cubejs-backend-native/src/python/cube_config.rs @@ -2,7 +2,7 @@ use convert_case::{Case, Casing}; use neon::prelude::*; use pyo3::{PyAny, PyResult}; -use crate::cross::{CLRepr, CLReprObject, CLReprPython}; +use crate::cross::{CLRepr, CLReprObject, CLReprObjectKind}; pub struct CubeConfigPy { properties: CLReprObject, @@ -11,7 +11,7 @@ pub struct CubeConfigPy { impl CubeConfigPy { pub fn new() -> Self { Self { - properties: CLReprObject::new(), + properties: CLReprObject::new(CLReprObjectKind::Object), } } diff --git a/packages/cubejs-backend-native/src/python/entry.rs b/packages/cubejs-backend-native/src/python/entry.rs index 9e8eed0eea696..b780b1f689533 100644 --- a/packages/cubejs-backend-native/src/python/entry.rs +++ b/packages/cubejs-backend-native/src/python/entry.rs @@ -77,9 +77,9 @@ fn python_load_model(mut cx: FunctionContext) -> JsResult { let model_module = PyModule::from_code(py, &model_content, &model_file_name, "")?; - let mut collected_functions = CLReprObject::new(); - let mut collected_variables = CLReprObject::new(); - let mut collected_filters = CLReprObject::new(); + let mut collected_functions = CLReprObject::new(CLReprObjectKind::Object); + let mut collected_variables = CLReprObject::new(CLReprObjectKind::Object); + let mut collected_filters = CLReprObject::new(CLReprObjectKind::Object); if model_module.hasattr("template")? { let template = model_module.getattr("template")?; diff --git a/packages/cubejs-backend-native/src/python/python_model.rs b/packages/cubejs-backend-native/src/python/python_model.rs index a30ec54d4be34..cbc9bdd02cc32 100644 --- a/packages/cubejs-backend-native/src/python/python_model.rs +++ b/packages/cubejs-backend-native/src/python/python_model.rs @@ -1,6 +1,6 @@ use neon::prelude::*; -use crate::cross::{CLRepr, CLReprObject}; +use crate::cross::{CLRepr, CLReprObject, CLReprObjectKind}; pub struct CubePythonModel { functions: CLReprObject, @@ -23,7 +23,7 @@ impl Finalize for CubePythonModel {} impl CubePythonModel { #[allow(clippy::wrong_self_convention)] pub fn to_object<'a, C: Context<'a>>(self, cx: &mut C) -> JsResult<'a, JsValue> { - let mut obj = CLReprObject::new(); + let mut obj = CLReprObject::new(CLReprObjectKind::Object); obj.insert("functions".to_string(), CLRepr::Object(self.functions)); obj.insert("variables".to_string(), CLRepr::Object(self.variables)); obj.insert("filters".to_string(), CLRepr::Object(self.filters)); diff --git a/packages/cubejs-backend-native/src/python/runtime.rs b/packages/cubejs-backend-native/src/python/runtime.rs index 7836eca591c21..3a0308243a33e 100644 --- a/packages/cubejs-backend-native/src/python/runtime.rs +++ b/packages/cubejs-backend-native/src/python/runtime.rs @@ -1,4 +1,4 @@ -use crate::cross::{CLRepr, CLReprPython}; +use crate::cross::CLRepr; use crate::python::neon_py::*; use crate::tokio_runtime_node; use cubesql::CubeError; @@ -95,14 +95,19 @@ impl PyRuntime { let (fun, args, callback) = task.split(); let task_result = Python::with_gil(move |py| -> PyResult { - let mut args_tuple = Vec::with_capacity(args.len()); + let mut prep_tuple = Vec::with_capacity(args.len()); + let mut py_kwargs = None; for arg in args { - args_tuple.push(arg.into_py(py)?); + if arg.is_kwarg() { + py_kwargs = Some(arg.into_py_dict(py)?); + } else { + prep_tuple.push(arg.into_py(py)?); + } } - let args = PyTuple::new(py, args_tuple); - let call_res = fun.call1(py, args)?; + let py_args = PyTuple::new(py, prep_tuple); + let call_res = fun.call(py, py_args, py_kwargs)?; let is_coroutine = unsafe { pyo3::ffi::PyCoro_CheckExact(call_res.as_ptr()) == 1 }; if is_coroutine { diff --git a/packages/cubejs-backend-native/src/template/entry.rs b/packages/cubejs-backend-native/src/template/entry.rs index f56a14b7a994f..212e12327afbc 100644 --- a/packages/cubejs-backend-native/src/template/entry.rs +++ b/packages/cubejs-backend-native/src/template/entry.rs @@ -110,7 +110,7 @@ impl JinjaEngine { let template_compile_context = CLRepr::from_js_ref(cx.argument::(1)?, &mut cx)?; let template_python_context = CLRepr::from_js_ref(cx.argument::(2)?, &mut cx)?; - let mut to_jinja_ctx = CLReprObject::new(); + let mut to_jinja_ctx = CLReprObject::new(CLReprObjectKind::Object); to_jinja_ctx.insert("COMPILE_CONTEXT".to_string(), template_compile_context); if !template_python_context.is_null() { diff --git a/packages/cubejs-backend-native/src/template/mj_value/python.rs b/packages/cubejs-backend-native/src/template/mj_value/python.rs index 9fcd8783f728d..510b121fa0c9a 100644 --- a/packages/cubejs-backend-native/src/template/mj_value/python.rs +++ b/packages/cubejs-backend-native/src/template/mj_value/python.rs @@ -64,7 +64,11 @@ pub fn from_minijinja_value(from: &mjv::Value) -> Result { Ok(CLRepr::Array(arr)) } mjv::ValueKind::Map => { - let mut obj = CLReprObject::new(); + let mut obj = CLReprObject::new(if from.is_kwargs() { + CLReprObjectKind::KWargs + } else { + CLReprObjectKind::Object + }); for key in from.try_iter()? { let value = if let Ok(v) = from.get_item(&key) { diff --git a/packages/cubejs-backend-native/test/__snapshots__/jinja.test.ts.snap b/packages/cubejs-backend-native/test/__snapshots__/jinja.test.ts.snap index c278af3fc34ed..85cf706de4e07 100644 --- a/packages/cubejs-backend-native/test/__snapshots__/jinja.test.ts.snap +++ b/packages/cubejs-backend-native/test/__snapshots__/jinja.test.ts.snap @@ -185,7 +185,12 @@ exports[`Jinja (new api) render arguments-test.yml.jinja: arguments-test.yml.jin arg_seq_1: [1,2,3,4,5] arg_seq_2: [5,4,3,2,1] arg_sum_tuple: 3 - arg_sum_map: 20" + arg_sum_map: 20 + arg_kwargs1: \\"arg1: first value, arg2: second value, kwarg:(three=3 arg)\\" + arg_kwargs2: \\"arg1: first value, arg2: second value, kwarg:(four=4 arg,three=3 arg)\\" + arg_kwargs3: \\"arg1: first value, arg2: second value, kwarg:(five=5 arg,four=4 arg,three=3 arg)\\" + arg_named_arguments1: \\"arg1: 1 arg, arg2: 2 arg\\" + arg_named_arguments2: \\"arg1: 1 arg, arg2: 2 arg\\"" `; exports[`Jinja (new api) render data-model.yml.jinja: data-model.yml.jinja 1`] = ` @@ -249,7 +254,7 @@ dump: exports[`Jinja (new api) render template_error_python.jinja: template_error_python.jinja 1`] = ` [Error: could not render block: Call error: Python error: Exception: Random Exception Traceback (most recent call last): - File "jinja-instance.py", line 110, in throw_exception + File "jinja-instance.py", line 120, in throw_exception ------------------------- template_error_python.jinja ------------------------- 3 | 3 diff --git a/packages/cubejs-backend-native/test/jinja.test.ts b/packages/cubejs-backend-native/test/jinja.test.ts index 6a837070a84ad..d1a4e589b50a6 100644 --- a/packages/cubejs-backend-native/test/jinja.test.ts +++ b/packages/cubejs-backend-native/test/jinja.test.ts @@ -91,6 +91,8 @@ suite('Python model', () => { load_data: expect.any(Object), load_data_sync: expect.any(Object), arg_bool: expect.any(Object), + arg_kwargs: expect.any(Object), + arg_named_arguments: expect.any(Object), arg_sum_integers: expect.any(Object), arg_str: expect.any(Object), arg_null: expect.any(Object), @@ -125,6 +127,8 @@ darwinSuite('Scope Python model', () => { load_data: expect.any(Object), load_data_sync: expect.any(Object), arg_bool: expect.any(Object), + arg_kwargs: expect.any(Object), + arg_named_arguments: expect.any(Object), arg_sum_integers: expect.any(Object), arg_str: expect.any(Object), arg_null: expect.any(Object), diff --git a/packages/cubejs-backend-native/test/python.test.ts b/packages/cubejs-backend-native/test/python.test.ts index 3fd261c894afd..0d2f08768d518 100644 --- a/packages/cubejs-backend-native/test/python.test.ts +++ b/packages/cubejs-backend-native/test/python.test.ts @@ -213,6 +213,7 @@ darwinSuite('Old Python Config', () => { schemaPath: 'models', telemetry: false, contextToApiScopes: expect.any(Function), + extendContext: expect.any(Function), logger: expect.any(Function), pgSqlPort: 5555, preAggregationsSchema: expect.any(Function), diff --git a/packages/cubejs-backend-native/test/templates/arguments-test.yml.jinja b/packages/cubejs-backend-native/test/templates/arguments-test.yml.jinja index 7d96023c37c68..36cd13873bacb 100644 --- a/packages/cubejs-backend-native/test/templates/arguments-test.yml.jinja +++ b/packages/cubejs-backend-native/test/templates/arguments-test.yml.jinja @@ -15,3 +15,8 @@ test: arg_seq_2: {{ arg_seq(my_sequence_2) }} arg_sum_tuple: {{ arg_sum_tuple(my_int_tuple) }} arg_sum_map: {{ arg_sum_map(my_map) }} + arg_kwargs1: {{ arg_kwargs("first value", "second value", three="3 arg") }} + arg_kwargs2: {{ arg_kwargs("first value", "second value", three="3 arg", four="4 arg") }} + arg_kwargs3: {{ arg_kwargs("first value", "second value", three="3 arg", four="4 arg", five="5 arg") }} + arg_named_arguments1: {{ arg_named_arguments(arg2="2 arg", arg1="1 arg") }} + arg_named_arguments2: {{ arg_named_arguments(arg1="1 arg", arg2="2 arg") }} diff --git a/packages/cubejs-backend-native/test/templates/jinja-instance.py b/packages/cubejs-backend-native/test/templates/jinja-instance.py index 8d23bb107c777..ac91f6fb35818 100644 --- a/packages/cubejs-backend-native/test/templates/jinja-instance.py +++ b/packages/cubejs-backend-native/test/templates/jinja-instance.py @@ -33,6 +33,16 @@ def arg_sum_tuple(tu): def arg_sum_map(obj): return obj['field_a'] + obj['field_b'] +@template.function +def arg_kwargs(arg1, arg2, **kwargs): + kwargs_str = ",".join(f"{key}={value}" for key, value in sorted(kwargs.items())) + + return "arg1: " + arg1 + ", arg2: " + arg2 + ", kwarg:(" + kwargs_str + ")" + +@template.function +def arg_named_arguments(arg1, arg2): + return "arg1: " + arg1 + ", arg2: " + arg2 + @template.function def arg_seq(a): return a diff --git a/packages/cubejs-backend-native/test/templates/scoped-utils.py b/packages/cubejs-backend-native/test/templates/scoped-utils.py index 8bb9e7ddec021..9808728108a1e 100644 --- a/packages/cubejs-backend-native/test/templates/scoped-utils.py +++ b/packages/cubejs-backend-native/test/templates/scoped-utils.py @@ -25,6 +25,16 @@ def arg_sum_tuple(tu): def arg_sum_map(obj): return obj['field_a'] + obj['field_b'] +@context_func +def arg_kwargs(**kwargs): + kwargs_str = ",".join(f"{key}={value}" for key, value in sorted(kwargs.items())) + + return "arg1: " + arg1 + ", arg2: " + arg2 + ", kwarg:(" + kwargs_str + ")" + +@context_func +def arg_named_arguments(arg1, arg2): + return "arg1: " + arg1 + ", arg2: " + arg2 + @context_func def arg_seq(a): return a