Skip to content

Commit db70ed2

Browse files
ovrovr
committed
fix(native): Jinja - pass kwargs correctly into Python
1 parent cd90d56 commit db70ed2

File tree

15 files changed

+102
-40
lines changed

15 files changed

+102
-40
lines changed

‎packages/cubejs-backend-native/src/cross/clrepr.rs‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,24 @@ use std::collections::hash_map::{IntoIter, Iter, Keys};
1515
use std::collections::HashMap;
1616
use std::sync::Arc;
1717

18+
#[derive(Debug, Clone)]
19+
pub enum CLReprObjectKind {
20+
Object,
21+
KWargs,
22+
}
23+
1824
#[derive(Clone)]
19-
pub struct CLReprObject(pub(crate) HashMap<String, CLRepr>);
25+
pub struct CLReprObject(pub(crate) HashMap<String, CLRepr>, CLReprObjectKind);
2026

2127
impl Default for CLReprObject {
2228
fn default() -> Self {
23-
Self::new()
29+
Self::new(CLReprObjectKind::Object)
2430
}
2531
}
2632

2733
impl CLReprObject {
28-
pub fn new() -> Self {
29-
Self(HashMap::new())
34+
pub fn new(kind: CLReprObjectKind) -> Self {
35+
Self(HashMap::new(), kind)
3036
}
3137

3238
pub fn get(&self, key: &str) -> Option<&CLRepr> {
@@ -110,6 +116,18 @@ pub enum CLRepr {
110116
Null,
111117
}
112118

119+
impl CLRepr {
120+
pub fn is_kwarg(&self) -> bool {
121+
match self {
122+
CLRepr::Object(obj) => match obj.1 {
123+
CLReprObjectKind::KWargs => true,
124+
_ => false,
125+
},
126+
_ => false,
127+
}
128+
}
129+
}
130+
113131
impl std::fmt::Display for CLRepr {
114132
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
115133
std::fmt::Display::fmt(&self, f)
@@ -179,7 +197,7 @@ impl CLRepr {
179197

180198
Ok(CLRepr::Array(r))
181199
} else if from.is_a::<JsObject, _>(cx) {
182-
let mut obj = CLReprObject::new();
200+
let mut obj = CLReprObject::new(CLReprObjectKind::Object);
183201

184202
let v = from.downcast_or_throw::<JsObject, _>(cx)?;
185203
let properties = v.get_own_property_names(cx)?;

‎packages/cubejs-backend-native/src/cross/clrepr_python.rs‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use crate::cross::clrepr::CLReprObject;
2-
use crate::cross::{CLRepr, StringType};
2+
use crate::cross::{CLRepr, CLReprObjectKind, StringType};
33
use pyo3::exceptions::{PyNotImplementedError, PyTypeError};
44
use pyo3::types::{
55
PyBool, PyComplex, PyDate, PyDict, PyFloat, PyFrame, PyFunction, PyInt, PyList, PySequence,
@@ -16,15 +16,9 @@ pub enum PythonRef {
1616
PyExternalFunction(Py<PyFunction>),
1717
}
1818

19-
pub trait CLReprPython: Sized {
20-
fn from_python_ref(v: &PyAny) -> Result<Self, PyErr>;
21-
fn into_py_impl(from: CLRepr, py: Python) -> Result<PyObject, PyErr>;
22-
fn into_py(self, py: Python) -> Result<PyObject, PyErr>;
23-
}
24-
25-
impl CLReprPython for CLRepr {
19+
impl CLRepr {
2620
/// Convert python value to CLRepr
27-
fn from_python_ref(v: &PyAny) -> Result<Self, PyErr> {
21+
pub fn from_python_ref(v: &PyAny) -> Result<Self, PyErr> {
2822
if v.is_none() {
2923
return Ok(Self::Null);
3024
}
@@ -47,7 +41,7 @@ impl CLReprPython for CLRepr {
4741
Self::Int(i)
4842
} else if v.get_type().is_subclass_of::<PyDict>()? {
4943
let d = v.downcast::<PyDict>()?;
50-
let mut obj = CLReprObject::new();
44+
let mut obj = CLReprObject::new(CLReprObjectKind::Object);
5145

5246
for (k, v) in d.iter() {
5347
if k.get_type().is_subclass_of::<PyString>()? {
@@ -126,6 +120,16 @@ impl CLReprPython for CLRepr {
126120
})
127121
}
128122

123+
fn into_py_dict_impl(obj: CLReprObject, py: Python) -> Result<&PyDict, PyErr> {
124+
let r = PyDict::new(py);
125+
126+
for (k, v) in obj.into_iter() {
127+
r.set_item(k, Self::into_py_impl(v, py)?)?;
128+
}
129+
130+
Ok(r)
131+
}
132+
129133
fn into_py_impl(from: CLRepr, py: Python) -> Result<PyObject, PyErr> {
130134
Ok(match from {
131135
CLRepr::String(v, _) => PyString::new(py, &v).to_object(py),
@@ -155,11 +159,7 @@ impl CLReprPython for CLRepr {
155159
PyTuple::new(py, elements).to_object(py)
156160
}
157161
CLRepr::Object(obj) => {
158-
let r = PyDict::new(py);
159-
160-
for (k, v) in obj.into_iter() {
161-
r.set_item(k, Self::into_py_impl(v, py)?)?;
162-
}
162+
let r = Self::into_py_dict_impl(obj, py)?;
163163

164164
r.to_object(py)
165165
}
@@ -189,7 +189,18 @@ impl CLReprPython for CLRepr {
189189
})
190190
}
191191

192-
fn into_py(self, py: Python) -> Result<PyObject, PyErr> {
192+
pub fn into_py_dict(self, py: Python) -> Result<&PyDict, PyErr> {
193+
Ok(match self {
194+
CLRepr::Object(obj) => Self::into_py_dict_impl(obj, py)?,
195+
other => {
196+
return Err(PyErr::new::<PyNotImplementedError, _>(
197+
"Unable to represent JsFunction in Python",
198+
))
199+
}
200+
})
201+
}
202+
203+
pub fn into_py(self, py: Python) -> Result<PyObject, PyErr> {
193204
Self::into_py_impl(self, py)
194205
}
195206
}

‎packages/cubejs-backend-native/src/cross/mod.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ mod clrepr_python;
44
#[cfg(feature = "python")]
55
mod py_in_js;
66

7-
pub use clrepr::{CLRepr, CLReprKind, CLReprObject, StringType};
7+
pub use clrepr::{CLRepr, CLReprKind, CLReprObject, CLReprObjectKind, StringType};
88

99
#[cfg(feature = "python")]
10-
pub use clrepr_python::{CLReprPython, PythonRef};
10+
pub use clrepr_python::PythonRef;

‎packages/cubejs-backend-native/src/python/cube_config.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use convert_case::{Case, Casing};
22
use neon::prelude::*;
33
use pyo3::{PyAny, PyResult};
44

5-
use crate::cross::{CLRepr, CLReprObject, CLReprPython};
5+
use crate::cross::{CLRepr, CLReprObject, CLReprObjectKind};
66

77
pub struct CubeConfigPy {
88
properties: CLReprObject,
@@ -11,7 +11,7 @@ pub struct CubeConfigPy {
1111
impl CubeConfigPy {
1212
pub fn new() -> Self {
1313
Self {
14-
properties: CLReprObject::new(),
14+
properties: CLReprObject::new(CLReprObjectKind::Object),
1515
}
1616
}
1717

‎packages/cubejs-backend-native/src/python/entry.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,9 @@ fn python_load_model(mut cx: FunctionContext) -> JsResult<JsPromise> {
7777

7878
let model_module = PyModule::from_code(py, &model_content, &model_file_name, "")?;
7979

80-
let mut collected_functions = CLReprObject::new();
81-
let mut collected_variables = CLReprObject::new();
82-
let mut collected_filters = CLReprObject::new();
80+
let mut collected_functions = CLReprObject::new(CLReprObjectKind::Object);
81+
let mut collected_variables = CLReprObject::new(CLReprObjectKind::Object);
82+
let mut collected_filters = CLReprObject::new(CLReprObjectKind::Object);
8383

8484
if model_module.hasattr("template")? {
8585
let template = model_module.getattr("template")?;

‎packages/cubejs-backend-native/src/python/python_model.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use neon::prelude::*;
22

3-
use crate::cross::{CLRepr, CLReprObject};
3+
use crate::cross::{CLRepr, CLReprObject, CLReprObjectKind};
44

55
pub struct CubePythonModel {
66
functions: CLReprObject,
@@ -23,7 +23,7 @@ impl Finalize for CubePythonModel {}
2323
impl CubePythonModel {
2424
#[allow(clippy::wrong_self_convention)]
2525
pub fn to_object<'a, C: Context<'a>>(self, cx: &mut C) -> JsResult<'a, JsValue> {
26-
let mut obj = CLReprObject::new();
26+
let mut obj = CLReprObject::new(CLReprObjectKind::Object);
2727
obj.insert("functions".to_string(), CLRepr::Object(self.functions));
2828
obj.insert("variables".to_string(), CLRepr::Object(self.variables));
2929
obj.insert("filters".to_string(), CLRepr::Object(self.filters));

‎packages/cubejs-backend-native/src/python/runtime.rs‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use crate::cross::{CLRepr, CLReprPython};
1+
use crate::cross::CLRepr;
22
use crate::python::neon_py::*;
33
use crate::tokio_runtime_node;
44
use cubesql::CubeError;
@@ -7,7 +7,7 @@ use neon::prelude::*;
77
use neon::types::Deferred;
88
use once_cell::sync::OnceCell;
99
use pyo3::prelude::*;
10-
use pyo3::types::{PyFunction, PyTuple};
10+
use pyo3::types::{PyDict, PyFunction, PyTuple};
1111
use std::fmt::Formatter;
1212
use std::future::Future;
1313
use std::pin::Pin;
@@ -95,14 +95,20 @@ impl PyRuntime {
9595
let (fun, args, callback) = task.split();
9696

9797
let task_result = Python::with_gil(move |py| -> PyResult<PyScheduledFunResult> {
98-
let mut args_tuple = Vec::with_capacity(args.len());
98+
let mut prep_tuple = Vec::with_capacity(args.len());
99+
let mut py_kwargs = None;
99100

100101
for arg in args {
101-
args_tuple.push(arg.into_py(py)?);
102+
if arg.is_kwarg() {
103+
let as_py = arg.into_py_dict(py)?;
104+
py_kwargs = Some(as_py);
105+
} else {
106+
prep_tuple.push(arg.into_py(py)?);
107+
}
102108
}
103109

104-
let args = PyTuple::new(py, args_tuple);
105-
let call_res = fun.call1(py, args)?;
110+
let py_args = PyTuple::new(py, prep_tuple);
111+
let call_res = fun.call(py, py_args, py_kwargs)?;
106112

107113
let is_coroutine = unsafe { pyo3::ffi::PyCoro_CheckExact(call_res.as_ptr()) == 1 };
108114
if is_coroutine {

‎packages/cubejs-backend-native/src/template/entry.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ impl JinjaEngine {
110110
let template_compile_context = CLRepr::from_js_ref(cx.argument::<JsValue>(1)?, &mut cx)?;
111111
let template_python_context = CLRepr::from_js_ref(cx.argument::<JsValue>(2)?, &mut cx)?;
112112

113-
let mut to_jinja_ctx = CLReprObject::new();
113+
let mut to_jinja_ctx = CLReprObject::new(CLReprObjectKind::Object);
114114
to_jinja_ctx.insert("COMPILE_CONTEXT".to_string(), template_compile_context);
115115

116116
if !template_python_context.is_null() {

‎packages/cubejs-backend-native/src/template/mj_value/python.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,11 @@ pub fn from_minijinja_value(from: &mjv::Value) -> Result<CLRepr, mj::Error> {
6464
Ok(CLRepr::Array(arr))
6565
}
6666
mjv::ValueKind::Map => {
67-
let mut obj = CLReprObject::new();
67+
let mut obj = CLReprObject::new(if from.is_kwargs() {
68+
CLReprObjectKind::KWargs
69+
} else {
70+
CLReprObjectKind::Object
71+
});
6872

6973
for key in from.try_iter()? {
7074
let value = if let Ok(v) = from.get_item(&key) {

‎packages/cubejs-backend-native/test/__snapshots__/jinja.test.ts.snap‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,8 @@ exports[`Jinja (new api) render arguments-test.yml.jinja: arguments-test.yml.jin
185185
arg_seq_1: [1,2,3,4,5]
186186
arg_seq_2: [5,4,3,2,1]
187187
arg_sum_tuple: 3
188-
arg_sum_map: 20"
188+
arg_sum_map: 20
189+
arg_kwargs: \\"arg1: first value, arg2: second value, kwarg:(3 arg4 arg)\\""
189190
`;
190191

191192
exports[`Jinja (new api) render data-model.yml.jinja: data-model.yml.jinja 1`] = `
@@ -249,7 +250,7 @@ dump:
249250
exports[`Jinja (new api) render template_error_python.jinja: template_error_python.jinja 1`] = `
250251
[Error: could not render block: Call error: Python error: Exception: Random Exception
251252
Traceback (most recent call last):
252-
File "jinja-instance.py", line 110, in throw_exception
253+
File "jinja-instance.py", line 119, in throw_exception
253254
254255
------------------------- template_error_python.jinja -------------------------
255256
3 | 3

0 commit comments

Comments
 (0)