Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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,142 @@ 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)
}

pub fn as_str(&self) -> Option<&str> {
match self {
FilterValue::Str(s) => Some(s.as_str()),
_ => None,
}
}

/// 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,
}
}

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 +166,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
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 @@ -34,21 +35,24 @@ impl<'a> FilterSqlContext<'a> {

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

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
@@ -1,4 +1,5 @@
use super::{FilterOperationSql, FilterSqlContext};
use crate::cube_bridge::base_query_options::FilterValue;
use crate::planner::filter::operators::like::LikeOp;
use cubenativeutils::CubeError;

Expand All @@ -8,7 +9,7 @@ impl FilterOperationSql for LikeOp {
&self
.values
.iter()
.map(|v| Some(v.clone()))
.map(|v| FilterValue::Str(v.clone()))
.collect::<Vec<_>>(),
&self.member_type,
)?;
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::filter_operator::FilterOperator;
use super::typed_filter::{resolve_base_symbol, TypedFilter};
use crate::cube_bridge::base_query_options::FilterValue;
use crate::planner::Compiler;
use crate::planner::MemberSymbol;
use cubenativeutils::CubeError;
Expand Down Expand Up @@ -39,7 +40,7 @@ impl BaseFilter {
member_evaluator: Rc<MemberSymbol>,
filter_type: FilterType,
filter_operator: FilterOperator,
values: Option<Vec<Option<String>>>,
values: Option<Vec<FilterValue>>,
compiler: Option<&mut Compiler>,
) -> Result<Rc<Self>, CubeError> {
let typed_filter = TypedFilter::builder()
Expand All @@ -58,7 +59,7 @@ impl BaseFilter {
pub fn change_operator(
&self,
filter_operator: FilterOperator,
values: Vec<Option<String>>,
values: Vec<FilterValue>,
use_raw_values: bool,
query_tools: Rc<crate::planner::query_tools::QueryTools>,
compiler: Option<&mut Compiler>,
Expand Down Expand Up @@ -129,7 +130,7 @@ impl BaseFilter {
}
}

pub fn values(&self) -> &Vec<Option<String>> {
pub fn values(&self) -> &Vec<FilterValue> {
self.typed_filter.values()
}

Expand Down Expand Up @@ -175,8 +176,7 @@ impl BaseFilter {
self.typed_filter
.values()
.iter()
.cloned()
.filter_map(|v| v)
.filter_map(|v| v.to_param_string())
.collect_vec(),
)
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::base_filter::{BaseFilter, FilterType};
use super::FilterOperator;
use crate::cube_bridge::base_query_options::FilterItem as NativeFilterItem;
use crate::cube_bridge::base_query_options::{FilterItem as NativeFilterItem, FilterValue};
use crate::planner::filter::{FilterGroup, FilterGroupOperator, FilterItem};
use crate::planner::query_tools::QueryTools;
use crate::planner::{Compiler, MemberSymbol};
Expand Down Expand Up @@ -58,7 +58,7 @@ impl<'a> FilterCompiler<'a> {
item.clone(),
FilterType::Dimension,
FilterOperator::InDateRange,
Some(date_range.into_iter().map(|v| Some(v)).collect()),
Some(date_range.into_iter().map(FilterValue::Str).collect()),
None,
)?;
self.time_dimension_filters.push(FilterItem::Item(filter));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ impl DebugSql for BaseFilter {
let values_str = self
.values()
.iter()
.map(|v| match v {
.map(|v| match v.to_param_string() {
Some(val) => format!("'{}'", val),
None => "NULL".to_string(),
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
})
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use crate::cube_bridge::base_query_options::FilterValue;

#[derive(Clone, Debug)]
pub enum ComparisonKind {
Gt,
Expand All @@ -11,12 +13,12 @@ pub enum ComparisonKind {
#[derive(Clone, Debug)]
pub struct ComparisonOp {
pub(crate) kind: ComparisonKind,
pub(crate) value: String,
pub(crate) value: FilterValue,
pub(crate) member_type: Option<String>,
}

impl ComparisonOp {
pub fn new(kind: ComparisonKind, value: String, member_type: Option<String>) -> Self {
pub fn new(kind: ComparisonKind, value: FilterValue, member_type: Option<String>) -> Self {
Self {
kind,
value,
Expand Down
Loading
Loading