Skip to content

fix(tesseract): preserve FILTER_PARAMS pushdown with segments #10608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hank-sq wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(tesseract): preserve FILTER_PARAMS pushdown with segments #10608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 47 additions & 0 deletions packages/cubejs-schema-compiler/test/integration/postgres/sql-generation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5708,4 +5708,51 @@ cubes:
},
]);
});

if (getEnv('nativeSqlPlanner')) {
describe('FILTER_PARAMS with segments under Tesseract', () => {
const fpCompilers = prepareJsCompiler(`
cube('orders_fp', {
sql: \`
SELECT * FROM orders
WHERE \${FILTER_PARAMS.orders_fp.created_at.filter('created_at')}
AND \${FILTER_PARAMS.orders_fp.status.filter('status')}
\`,
measures: {
count: { type: 'count' },
},
dimensions: {
id: { sql: 'id', type: 'number', primaryKey: true },
status: { sql: 'status', type: 'string' },
created_at: { sql: 'created_at', type: 'time' },
},
segments: {
completed: { sql: \`\${CUBE}.status = 'completed'\` },
},
});
`);

it('FILTER_PARAMS pushdown is preserved when a segment is present', async () => {
await fpCompilers.compiler.compile();
const query = new PostgresQuery(fpCompilers, {
measures: ['orders_fp.count'],
segments: ['orders_fp.completed'],
filters: [
{ member: 'orders_fp.status', operator: 'equals', values: ['completed'] },
],
timeDimensions: [{
dimension: 'orders_fp.created_at',
dateRange: ['2024-01-01', '2024-01-31'],
}],
timezone: 'UTC',
});

const [sql] = query.buildSqlAndParams();

expect(sql).not.toMatch(/WHERE\s+1\s*=\s*1\s+AND\s+1\s*=\s*1/);
expect(sql).toMatch(/created_at/);
expect(sql).toMatch(/status/);
});
});
}
});
130 changes: 64 additions & 66 deletions rust/cubesqlplanner/cubesqlplanner/src/plan/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ use cubenativeutils::CubeError;
use std::fmt;
use std::rc::Rc;

/// Whether a recursive `find_subtree_for_members` call matched every node
/// without pruning anything (segments, non-matching items, etc.).
type FullMatch = bool;

#[derive(Clone, PartialEq)]
pub enum FilterGroupOperator {
Or,
Expand Down Expand Up @@ -128,88 +132,82 @@ impl FilterItem {
}
}

/// Extract all member symbols from this filter tree
/// Returns None if filter tree is invalid (e.g., empty group)
/// Returns Some(set) with all member symbols found in the tree
fn extract_filter_members(&self) -> Option<Vec<Rc<MemberSymbol>>> {
match self {
FilterItem::Group(group) => {
// Empty groups are considered invalid
if group.items.is_empty() {
return None;
}

let mut all_members = Vec::new();

// Recursively extract from all children
for child in &group.items {
match child.extract_filter_members() {
None => return None, // If any child is invalid, entire tree is invalid
Some(mut members) => all_members.append(&mut members),
}
}

Some(all_members)
}
FilterItem::Item(item) => Some(vec![item.member_evaluator().clone()]),
FilterItem::Segment(_) => None,
}
}

/// Find subtree of filters that only contains filters for the specified members
/// Returns None if no matching filters found
/// Returns Some(FilterItem) with the subtree containing only filters for target members
/// Find subtree of filters that only contains filters for the specified members.
/// Returns `None` if no matching filters found.
/// Returns `Some(FilterItem)` with the subtree containing only filters for target members.
///
/// Partial matching is only supported for AND groups. OR groups only match
/// if all of their children match the target members.
///
/// This only processes AND groups - OR groups are not supported and will return None
/// `Segment` nodes are skipped during extraction -- they do not prevent
/// sibling member filters from being collected in AND groups.
pub fn find_subtree_for_members(&self, target_members: &[&String]) -> Option<FilterItem> {
self.find_subtree_for_members_inner(target_members)
.map(|(filter_item, _)| filter_item)
}

fn find_subtree_for_members_inner(
&self,
target_members: &[&String],
) -> Option<(FilterItem, FullMatch)> {
match self {
FilterItem::Group(group) => {
// Empty groups return None
if group.items.is_empty() {
return None;
}

// Extract all members from this filter subtree
let filter_members = self.extract_filter_members()?;
match group.operator {
FilterGroupOperator::And => {
let mut matching_children = Vec::new();
let mut all_children_fully_match = true;

// Check if all members in this filter are in the target set
let all_members_match = filter_members.iter().all(|member| {
target_members.iter().any(|target| {
&&member.clone().resolve_reference_chain().full_name() == target
})
});
for child in &group.items {
match child.find_subtree_for_members_inner(target_members) {
Some((matching_child, child_fully_matched)) => {
matching_children.push(matching_child);
all_children_fully_match &= child_fully_matched;
}
None => all_children_fully_match = false,
}
}

if all_members_match {
// All members match - return this entire filter subtree
return Some(self.clone());
}
if matching_children.is_empty() {
return None;
}

// Only process AND groups for partial matching
if group.operator == FilterGroupOperator::And {
let matching_children: Vec<FilterItem> = group
.items
.iter()
.filter_map(|child| child.find_subtree_for_members(target_members))
.collect();
if all_children_fully_match {
// Every child matches, so preserve the original group shape.
return Some((self.clone(), true));
}

if matching_children.is_empty() {
return None;
}
if matching_children.len() == 1 {
// Single match - return it directly without wrapping.
return Some((matching_children.into_iter().next().unwrap(), false));
}

if matching_children.len() == 1 {
// Single match - return it directly without wrapping
return Some(matching_children.into_iter().next().unwrap());
// Multiple matches - wrap in a new AND group.
Some((
FilterItem::Group(Rc::new(FilterGroup::new(
FilterGroupOperator::And,
matching_children,
))),
false,
))
}
FilterGroupOperator::Or => {
// OR groups can only be preserved if every child matches.
for child in &group.items {
let (_, child_fully_matched) =
child.find_subtree_for_members_inner(target_members)?;
if !child_fully_matched {
return None;
}
}

// Multiple matches - wrap in new AND group
return Some(FilterItem::Group(Rc::new(FilterGroup::new(
FilterGroupOperator::And,
matching_children,
))));
Some((self.clone(), true))
}
}

// OR groups are not supported
None
}
FilterItem::Item(item) => {
let member = item.member_evaluator();
Expand All @@ -219,7 +217,7 @@ impl FilterItem {
.iter()
.any(|target| &&member.clone().resolve_reference_chain().full_name() == target)
{
Some(self.clone())
Some((self.clone(), true))
} else {
None
}
Expand Down
15 changes: 12 additions & 3 deletions rust/cubesqlplanner/cubesqlplanner/src/planner/sql_evaluator/sql_call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,17 @@ impl SqlCall {
let (filter_params, filter_groups, deps, context_values) =
self.prepare_template_params(visitor, node_processor, &query_tools, templates)?;

Self::substitute_template(
let result = Self::substitute_template(
template,
&deps,
&filter_params,
&filter_groups,
&context_values,
)
)?;
// Filter-params callbacks (e.g. FILTER_PARAMS inside a segment SQL)
// may produce results containing {arg:N} dep references that the
// first pass inserted verbatim. A second pass resolves them.
Self::substitute_template(&result, &deps, &[], &[], &[])
} else {
Err(CubeError::internal(
"SqlCall::eval called for function that returns string".to_string(),
Expand Down Expand Up @@ -194,7 +198,12 @@ impl SqlCall {
})
.collect::<Result<Vec<_>, _>>()?,
};
Ok(result)
// Filter-params callbacks may produce results containing {arg:N}
// dep references; resolve them with a second pass.
result
.into_iter()
.map(|s| Self::substitute_template(&s, &deps, &[], &[], &[]))
.collect()
}

pub fn is_owned_by_cube(&self) -> bool {
Expand Down
Loading
Loading