chore(tesseract): Fix issue with FILTER_PARAMS and issue with multi-stage behavior of non multi-stage members #9211

waralexrom · 2025-02-11T14:35:28Z

Check List

Tests have been run in packages where changes made if available
Linter has been run for changed code
Tests for the changes have been added if not covered yet
Docs have been added / updated if required

Issue Reference this PR resolves

[For example #12]

Description of Changes Made (if issue reference is not provided)

[Description goes here]

codecov · 2025-02-11T14:42:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.69%. Comparing base (40a4b8d) to head (4a80bc5).
Report is 5 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #9211   +/-   ##
=======================================
  Coverage   83.69%   83.69%           
=======================================
  Files         228      228           
  Lines       82016    82016           
=======================================
  Hits        68640    68640           
  Misses      13376    13376

Flag	Coverage Δ
cubesql	`83.69% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

KSDaemon

👍🏻 Left some questions and minor notes.

KSDaemon · 2025-02-27T11:39:56Z

rust/cubenativeutils/src/wrappers/serializer/serializer.rs

@@ -138,7 +138,7 @@ impl<IT: InnerTypes> ser::Serializer for NativeSerdeSerializer<IT> {
    }

    fn serialize_none(self) -> Result<Self::Ok, Self::Error> {
-        Ok(self.context.undefined()?)
+        Ok(self.context.null()?)


Are you sure null is correct? In JS it will result in smth like: myVarOrProperty: null instead of undefined.

Yes, I think it's justified here. In general, this change was made to normally pass parameters in filters, which can be either a string or null.

KSDaemon · 2025-02-27T11:42:03Z

packages/cubejs-schema-compiler/test/integration/postgres/sql-generation.test.ts

-      select * from visitor_checkins WHERE
-      \${FILTER_PARAMS.visitor_checkins.created_at.filter('created_at')} AND
-      \${FILTER_GROUP(FILTER_PARAMS.visitor_checkins.created_at.filter("(created_at - INTERVAL '3 DAY')"), FILTER_PARAMS.visitor_checkins.source.filter('source'))}
+      select visitor_checkins.* from visitor_checkins left join visitors on visitor_checkins.visitor_id = visitors.id WHERE


Can you explain the change? Could you tell me why you didn't create another cube/test or why this one doesn't suit your needs?

This is an fix of an incorrect data model for tests. SECURITY_CONTEXT.filter should work regardless of whether the cube has built a join for a particular query or not. Therefore, this filter must be written directly in the sql of the cube. This is one of the reasons why SECURITY_CONTEXT is deprecated. In the case of tesseract it led to the fact that in one of the tests fails, because tesseract knows how to omit unnecessary joins in subqueries. But this is not the error of tesseract, but of incorrect use of SECURITY_CONTEXT in the model.

KSDaemon · 2025-02-27T11:43:19Z

packages/cubejs-schema-compiler/test/integration/postgres/sql-generation.test.ts

@@ -2975,6 +3057,37 @@ describe('SQL Generation', () => {
    }]
  ));

+  if (getEnv('nativeSqlPlanner')) {


I think it would be more correct to mark the test as skipped in case of not using nativeSqlPlanner instead of just avoiding it all.

KSDaemon · 2025-02-27T11:46:20Z

rust/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage_query_planner.rs

+        if time_dimension.get_date_range().is_some() && result_granularity.is_some() {
+            let granularity = time_dimension.get_granularity().unwrap(); //FIXME remove this unwrap
+            let date_range = time_dimension.get_date_range().unwrap(); //FIXME remove this unwrap
+            let seria = self


Suggested change

let seria = self

let series = self

KSDaemon · 2025-02-27T11:49:14Z

rust/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage_query_planner.rs

+        let description = if childs.is_empty() || !has_multi_stage_members(&member, false)? {
            if has_multi_stage_members(&member, false)? {


is it correct to check the same condition has_multi_stage_members(&member, false) twice?

KSDaemon · 2025-02-27T11:53:43Z

rust/cubesqlplanner/cubesqlplanner/src/planner/sql_evaluator/sql_call.rs

+    fn filters_to_native_filter_item(
+        &self,
+        filter: Option<Filter>,
+    ) -> Option<Vec<NativeFilterItem>> {
+        if let Some(filter) = filter {
+            let mut res = Vec::new();
+            for item in filter.items.iter() {
+                res.push(self.filters_to_native_filter_item_impl(item));
+            }
+            Some(res)
+        } else {
+            None
+        }
+    }


I think this one may be rewritten in more rust-idiomatic style, smth like:

Suggested change

fn filters_to_native_filter_item(

&self,

filter: Option<Filter>,

) -> Option<Vec<NativeFilterItem>> {

if let Some(filter) = filter {

let mut res = Vec::new();

for item in filter.items.iter() {

res.push(self.filters_to_native_filter_item_impl(item));

}

Some(res)

} else {

None

}

}

fn filters_to_native_filter_item(

&self,

filter: Option<Filter>,

) -> Option<Vec<NativeFilterItem>> {

filter.map(|filter|

filter.items.iter()

.map(|item| self.filters_to_native_filter_item_impl(item))

.collect()

)

}

This is a very temporary solution that will be removed within a month, so I don't see the point in doing much styling here yet :)

KSDaemon · 2025-02-27T11:55:41Z

rust/cubesqlplanner/cubesqlplanner/src/planner/sql_evaluator/sql_call.rs

+                let mut native_items = Vec::new();
+                for itm in group.items.iter() {
+                    native_items.push(self.filters_to_native_filter_item_impl(itm));
+                }


Same here, more idiomatic style:

Suggested change

let mut native_items = Vec::new();

for itm in group.items.iter() {

native_items.push(self.filters_to_native_filter_item_impl(itm));

}

let native_items: Vec<_> = group.items

.iter()

.map(|itm| self.filters_to_native_filter_item_impl(itm))

.collect();

Some as above

…tage behavior of not multi-stage members

KSDaemon

👍🏻 LGTM!

waralexrom requested a review from a team as a code owner February 11, 2025 14:35

waralexrom requested a review from paveltiunov February 11, 2025 20:50

KSDaemon changed the title ~~chore(tesseract): Fix issue with FILTER_PARAMS and issue with multi-stage behavior of not multi-stage members~~ chore(tesseract): Fix issue with FILTER_PARAMS and issue with multi-stage behavior of non multi-stage members Feb 27, 2025

KSDaemon reviewed Feb 27, 2025

View reviewed changes

waralexrom added 4 commits March 2, 2025 16:14

chore(tesseract): Fix issue with FILTER_PARAMS and issue with multi-s…

d0208bc

…tage behavior of not multi-stage members

update

d7ca17d

fmt

9adab8b

update

4a80bc5

waralexrom force-pushed the tesseract-exclude-values-from-filter-params branch from 06ae94b to 4a80bc5 Compare March 2, 2025 15:49

KSDaemon approved these changes Mar 3, 2025

View reviewed changes

waralexrom merged commit 405a604 into master Mar 3, 2025
83 checks passed

waralexrom deleted the tesseract-exclude-values-from-filter-params branch March 3, 2025 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(tesseract): Fix issue with FILTER_PARAMS and issue with multi-stage behavior of non multi-stage members #9211

chore(tesseract): Fix issue with FILTER_PARAMS and issue with multi-stage behavior of non multi-stage members #9211

waralexrom commented Feb 11, 2025

codecov bot commented Feb 11, 2025 •

edited

Loading

KSDaemon left a comment •

edited

Loading

KSDaemon Feb 27, 2025

waralexrom Mar 2, 2025

KSDaemon Feb 27, 2025

waralexrom Feb 27, 2025 •

edited

Loading

KSDaemon Feb 27, 2025

KSDaemon Feb 27, 2025

waralexrom Mar 2, 2025

KSDaemon Feb 27, 2025

waralexrom Mar 2, 2025

KSDaemon Feb 27, 2025

waralexrom Mar 2, 2025

KSDaemon Feb 27, 2025

waralexrom Mar 2, 2025

KSDaemon left a comment

		let description = if childs.is_empty() \|\| !has_multi_stage_members(&member, false)? {
		if has_multi_stage_members(&member, false)? {

chore(tesseract): Fix issue with FILTER_PARAMS and issue with multi-stage behavior of non multi-stage members #9211

chore(tesseract): Fix issue with FILTER_PARAMS and issue with multi-stage behavior of non multi-stage members #9211

Conversation

waralexrom commented Feb 11, 2025

codecov bot commented Feb 11, 2025 • edited Loading

Codecov Report

KSDaemon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waralexrom Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KSDaemon left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 11, 2025 •

edited

Loading

KSDaemon left a comment •

edited

Loading

waralexrom Feb 27, 2025 •

edited

Loading