Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions rust/cube/cubenativeutils/src/wrappers/serializer/deserializer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,15 @@ impl<'de, IT: InnerTypes> Deserializer<'de> for NativeSerdeDeserializer<IT> {
} else if let Ok(val) = self.input.to_string() {
visitor.visit_string(val.value().unwrap())
} else if let Ok(val) = self.input.to_number() {
visitor.visit_i64(val.value().unwrap() as i64) //We deserialize float value in
//different methods
let num = val.value().unwrap();
// Preserve fractional numbers as floats; only integral values are
// narrowed to i64 (whole-number JS values are the common case, and
// self-describing consumers like FilterValue expect them as ints).
if num.fract() == 0.0 && num.is_finite() {
visitor.visit_i64(num as i64)
} else {
visitor.visit_f64(num)
Comment on lines 42 to +50

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This narrowing applies to every call to the native deserialize_any (not just FilterValue). Previously every JS number was forwarded as i64 and serde coerced it to whatever the target type expected; now integral values go to visit_i64 and fractional values to visit_f64.

Two latent risks worth a quick check:

  1. Any existing struct field deserialized via this path that was previously receiving truncated integer values from fractional JS numbers will now see the original f64. That's strictly an improvement for correct callers but could surface previously-silent data loss in callers that targeted i64/u64 and relied on the truncation.
  2. f64::NAN is !is_finite(), so it goes to visit_f64(NaN) — most serde targets reject this, which is probably desired but worth noting.

Adding a unit test in deserializer.rs covering the four shapes (int, float, NaN, very large integral) would lock the contract down.

}
} else if let Ok(val) = self.input.to_array() {
let deserializer = NativeSeqDeserializer::<IT>::new(val);
visitor.visit_seq(deserializer)
Expand All @@ -67,14 +74,21 @@ impl<'de, IT: InnerTypes> Deserializer<'de> for NativeSerdeDeserializer<IT> {

fn deserialize_enum<V>(
self,
_name: &'static str,
name: &'static str,
_variants: &'static [&'static str],
_visitor: V,
) -> Result<V::Value, Self::Error>
where
V: Visitor<'de>,
{
todo!()
// The native deserializer has no notion of serde's tagged enum
// representation, so a derived `Deserialize` on an enum can't work here.
// Implement `Deserialize` manually via `deserialize_any` instead (see
// `FilterValue` in cubesqlplanner for an example).
Err(NativeObjSerializerError::Message(format!(
"Deserializing enum `{name}` is not supported by the native deserializer; \
implement Deserialize manually via deserialize_any"
)))
}

fn deserialize_bytes<V>(self, _visitor: V) -> Result<V::Value, Self::Error>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,144 @@ use cubenativeutils::wrappers::serializer::{
};
use cubenativeutils::wrappers::{NativeArray, NativeContextHolder, NativeObjectHandle};
use cubenativeutils::CubeError;
use serde::{Deserialize, Serialize};
use serde::de::Visitor;
use serde::{Deserialize, Deserializer, Serialize, Serializer};
use std::any::Any;
use std::collections::HashMap;
use std::fmt;
use std::rc::Rc;

/// A single value of a filter (`equals`, `in`, `gt`, …).
#[derive(Debug, Clone, PartialEq)]
pub enum FilterValue {
Str(String),
Bool(bool),
Num(f64),
Null,
}

impl FilterValue {
pub fn is_null(&self) -> bool {
matches!(self, FilterValue::Null)
}

/// Canonical string representation bound as a SQL parameter. `Null` yields
/// `None` (the value is dropped / handled as `IS NULL`). Whole numbers are
/// rendered without a trailing `.0` (`42.0` → `"42"`).
pub fn to_param_string(&self) -> Option<String> {
Comment on lines +32 to +36

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small things on format_number:

  • format!("{}", n as i64) for the integral fast path silently saturates if n is outside i64::MIN..=i64::MAX. The n.abs() < 1e15 guard keeps us well inside that range — good — but it also excludes values like 1e15 itself even though they're exactly representable and fit in i64. Using n.abs() <= (1_i64 << 53) as f64 is the conventional cutoff (the largest f64 with unique integer round-trip) and matches what JS Number.isSafeInteger enforces upstream.
  • For the fallback format!("{}", n), the default Display for f64 uses minimum digits for round-tripping. That's fine for most values but won't match how the JS side originally stringified the number (e.g. JS may emit 1e-7 while Rust emits 0.0000001). If anything downstream compares the rendered param against an exact string, this can diverge. Probably out-of-scope for this PR, but worth a comment noting the format is "Rust default, not JS-compatible".

match self {
FilterValue::Str(s) => Some(s.clone()),
FilterValue::Bool(b) => Some(b.to_string()),
FilterValue::Num(n) => Some(Self::format_number(*n)),
FilterValue::Null => None,
}
}

pub fn to_debug_string(&self) -> String {
match self {
FilterValue::Str(s) => format!("'{}'", s),
FilterValue::Bool(b) => b.to_string(),
FilterValue::Num(n) => Self::format_number(*n),
FilterValue::Null => "NULL".to_string(),
}
}

fn format_number(n: f64) -> String {
if n.is_finite() && n.fract() == 0.0 && n.abs() < 1e15 {
format!("{}", n as i64)
} else {
format!("{}", n)
}
}
}

impl From<Option<String>> for FilterValue {
fn from(value: Option<String>) -> Self {
match value {
Some(s) => FilterValue::Str(s),
None => FilterValue::Null,
}
}
}

impl From<String> for FilterValue {
fn from(value: String) -> Self {
FilterValue::Str(value)
}
}

impl Serialize for FilterValue {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
match self {
FilterValue::Str(s) => serializer.serialize_str(s),
FilterValue::Bool(b) => serializer.serialize_bool(*b),
FilterValue::Num(n) => serializer.serialize_f64(*n),
FilterValue::Null => serializer.serialize_none(),
}
}
}

impl<'de> Deserialize<'de> for FilterValue {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
struct FilterValueVisitor;

impl<'de> Visitor<'de> for FilterValueVisitor {
type Value = FilterValue;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("a string, boolean, number, or null")
}

fn visit_bool<E>(self, v: bool) -> Result<Self::Value, E> {
Ok(FilterValue::Bool(v))
}

fn visit_i64<E>(self, v: i64) -> Result<Self::Value, E> {
Ok(FilterValue::Num(v as f64))
}

fn visit_u64<E>(self, v: u64) -> Result<Self::Value, E> {
Ok(FilterValue::Num(v as f64))
}

fn visit_f64<E>(self, v: f64) -> Result<Self::Value, E> {
Ok(FilterValue::Num(v))
}

fn visit_str<E>(self, v: &str) -> Result<Self::Value, E> {
Ok(FilterValue::Str(v.to_string()))
}

fn visit_string<E>(self, v: String) -> Result<Self::Value, E> {
Ok(FilterValue::Str(v))
}

fn visit_unit<E>(self) -> Result<Self::Value, E> {
Ok(FilterValue::Null)
}

fn visit_none<E>(self) -> Result<Self::Value, E> {
Ok(FilterValue::Null)
}

fn visit_some<D>(self, deserializer: D) -> Result<Self::Value, D::Error>
where
D: Deserializer<'de>,
{
deserializer.deserialize_any(self)
}
Comment thread
claude[bot] marked this conversation as resolved.
}

deserializer.deserialize_any(FilterValueVisitor)
}
}

#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct MaskedMemberItem {
pub member: String,
Expand All @@ -35,7 +168,7 @@ pub struct FilterItem {
pub member: Option<String>,
pub dimension: Option<String>,
pub operator: Option<String>,
pub values: Option<Vec<Option<String>>>,
pub values: Option<Vec<FilterValue>>,
}

#[derive(Serialize, Deserialize, Debug, Clone)]
Expand Down Expand Up @@ -107,3 +240,78 @@ pub trait BaseQueryOptions {
#[nbridge(field, optional, vec)]
fn join_hints(&self) -> Result<Option<Vec<JoinHintItem>>, CubeError>;
}

#[cfg(test)]
mod tests {
use super::FilterValue;

#[test]
fn deserializes_native_boolean() {
let values: Vec<FilterValue> = serde_json::from_str("[true, false]").unwrap();
assert_eq!(
values,
vec![FilterValue::Bool(true), FilterValue::Bool(false)]
);
}

#[test]
fn deserializes_native_integer() {
let values: Vec<FilterValue> = serde_json::from_str("[42]").unwrap();
assert_eq!(values, vec![FilterValue::Num(42.0)]);
}

#[test]
fn deserializes_native_fractional_number() {
let values: Vec<FilterValue> = serde_json::from_str("[42.5]").unwrap();
assert_eq!(values, vec![FilterValue::Num(42.5)]);
}

#[test]
fn deserializes_quoted_value_as_string() {
let values: Vec<FilterValue> = serde_json::from_str(r#"["42"]"#).unwrap();
assert_eq!(values, vec![FilterValue::Str("42".to_string())]);
}

#[test]
fn deserializes_null() {
let values: Vec<FilterValue> = serde_json::from_str("[null]").unwrap();
assert_eq!(values, vec![FilterValue::Null]);
}

#[test]
fn deserializes_mixed_value_array() {
let values: Vec<FilterValue> = serde_json::from_str(r#"[true, 42, "x", null]"#).unwrap();
assert_eq!(
values,
vec![
FilterValue::Bool(true),
FilterValue::Num(42.0),
FilterValue::Str("x".to_string()),
FilterValue::Null,
]
);
}

#[test]
fn to_param_string_renders_canonical_form() {
assert_eq!(
FilterValue::Bool(true).to_param_string().as_deref(),
Some("true")
);
assert_eq!(
FilterValue::Num(42.0).to_param_string().as_deref(),
Some("42")
);
assert_eq!(
FilterValue::Num(42.5).to_param_string().as_deref(),
Some("42.5")
);
assert_eq!(
FilterValue::Str("42".to_string())
.to_param_string()
.as_deref(),
Some("42")
);
assert_eq!(FilterValue::Null.to_param_string(), None);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use super::pre_aggregation_obj::{NativePreAggregationObj, PreAggregationObj};
use super::security_context::{NativeSecurityContext, SecurityContext};
use super::sql_templates_render::{NativeSqlTemplatesRender, SqlTemplatesRender};
use super::sql_utils::{NativeSqlUtils, SqlUtils};
use crate::cube_bridge::base_query_options::FilterValue;
use crate::cube_bridge::join_hints::JoinHintItem;
use cubenativeutils::wrappers::serializer::{
NativeDeserialize, NativeDeserializer, NativeSerialize,
Expand All @@ -24,7 +25,7 @@ pub trait BaseTools {
fn driver_tools(&self, external: bool) -> Result<Rc<dyn DriverTools>, CubeError>;
fn sql_templates(&self) -> Result<Rc<dyn SqlTemplatesRender>, CubeError>;
fn sql_utils_for_rust(&self) -> Result<Rc<dyn SqlUtils>, CubeError>;
fn get_allocated_params(&self) -> Result<Vec<String>, CubeError>;
fn get_allocated_params(&self) -> Result<Vec<FilterValue>, CubeError>;
fn all_cube_members(&self, path: String) -> Result<Vec<String>, CubeError>;
fn interval_and_minimal_time_unit(&self, interval: String) -> Result<Vec<String>, CubeError>;
fn get_pre_aggregation_by_name(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ pub fn pretty_print_filter_item(
item.filter_operator().to_string(),
item.values()
.iter()
.map(|v| v.clone().unwrap_or("null".to_string()))
.map(|v| v.to_param_string().unwrap_or_else(|| "null".to_string()))
.collect::<Vec<_>>()
.join(", ")
),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::cube_bridge::base_query_options::FilterValue;
use crate::planner::query_tools::QueryTools;
use crate::planner::sql_templates::{PlanSqlTemplates, TemplateProjectionColumn};
use crate::planner::QueryDateTimeHelper;
Expand Down Expand Up @@ -33,6 +34,17 @@ impl<'a> FilterSqlContext<'a> {
}

pub fn allocate_and_cast(
&self,
value: &FilterValue,
member_type: &Option<String>,
) -> Result<String, CubeError> {
let value = value.to_param_string().ok_or_else(|| {
CubeError::internal("Unexpected null value for a single-value filter".to_string())
})?;
self.allocate_and_cast_str(&value, member_type)
}

pub fn allocate_and_cast_str(
&self,
value: &str,
member_type: &Option<String>,
Expand All @@ -43,12 +55,12 @@ impl<'a> FilterSqlContext<'a> {

pub fn allocate_and_cast_values(
&self,
values: &[Option<String>],
values: &[FilterValue],
member_type: &Option<String>,
) -> Result<Vec<String>, CubeError> {
values
.iter()
.filter_map(|v| v.as_ref())
.filter(|v| !v.is_null())
.map(|v| self.allocate_and_cast(v, member_type))
.collect()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use cubenativeutils::CubeError;

impl FilterOperationSql for InListOp {
fn to_sql(&self, ctx: &FilterSqlContext) -> Result<String, CubeError> {
let has_null = self.values.iter().any(|v| v.is_none());
let has_null = self.values.iter().any(|v| v.is_null());
let need_null_check = if self.negated { !has_null } else { has_null };
let allocated = ctx.allocate_and_cast_values(&self.values, &self.member_type)?;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,11 @@ use cubenativeutils::CubeError;

impl FilterOperationSql for LikeOp {
fn to_sql(&self, ctx: &FilterSqlContext) -> Result<String, CubeError> {
let allocated = ctx.allocate_and_cast_values(
&self
.values
.iter()
.map(|v| Some(v.clone()))
.collect::<Vec<_>>(),
&self.member_type,
)?;
let allocated = self
.values
.iter()
.map(|v| ctx.allocate_and_cast_str(v, &self.member_type))
.collect::<Result<Vec<_>, _>>()?;

let like_parts = allocated
.into_iter()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,21 +78,22 @@ impl TypedFilter {
let from = self
.values()
.first()
.and_then(|v| v.as_ref())
.map(|v| ctx.format_and_allocate_from_date(v))
.and_then(|v| v.to_param_string())
.map(|v| ctx.format_and_allocate_from_date(&v))
.transpose()?;
let to = self
.values()
.get(1)
.and_then(|v| v.as_ref())
.map(|v| ctx.format_and_allocate_to_date(v))
.and_then(|v| v.to_param_string())
.map(|v| ctx.format_and_allocate_to_date(&v))
.transpose()?;
[from, to].into_iter().flatten().collect()
}
_ => self
.values()
.iter()
.filter_map(|v| v.as_ref().map(|v| query_tools.allocate_param(v)))
.filter_map(|v| v.to_param_string())
.map(|v| query_tools.allocate_param(&v))
.collect::<Vec<_>>(),
};
callback.call(&args)
Expand Down
Loading
Loading