feat(tesseract): Multi-stage filter directives#10827
Conversation
github-actions
Bot
added
rust
|
Claude finished @waralexrom's task in 3m 49s —— View job Review of PR #10827: Multi-stage filter directives
Overall AssessmentThis is a well-structured feature addition that introduces Code QualityStrengths:
Issues flagged (see inline comments):
SecurityNo security concerns. The PerformanceNo performance concerns. The filter directive is applied once per multi-stage node during planning (not per-row). The TestingTest coverage is thorough across all layers:
Good coverage of edge cases: empty groups bubbling up, nested empty groups, DocumentationThe TODO comments documenting known No user-facing documentation changes are included, which makes sense if this is behind the Tesseract flag ( SummaryClean feature with good test coverage and thoughtful architecture. The main actionable item is adding |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const MultiStageFilter = Joi.object().keys({ | ||
| mode: Joi.string().valid('relative', 'fixed'), | ||
| exclude: Joi.func(), | ||
| keepOnly: Joi.func(), | ||
| include: Joi.array().items( | ||
| MultiStageIncludeMemberFilterSchema, | ||
| MultiStageIncludeConditionSchema | ||
| ), | ||
| }); |
There was a problem hiding this comment.
Issue (medium): exclude and keepOnly mutual exclusivity not enforced at validation time.
The Rust side (build_filter in multi_stage.rs:170) correctly rejects schemas that specify both exclude and keepOnly, but this Joi schema allows both through. This means users will get a late, less-helpful error from the Rust planner instead of a clear schema validation error.
Consider adding .nand('exclude', 'keepOnly') here:
| const MultiStageFilter = Joi.object().keys({ | |
| mode: Joi.string().valid('relative', 'fixed'), | |
| exclude: Joi.func(), | |
| keepOnly: Joi.func(), | |
| include: Joi.array().items( | |
| MultiStageIncludeMemberFilterSchema, | |
| MultiStageIncludeConditionSchema | |
| ), | |
| }); | |
| const MultiStageFilter = Joi.object().keys({ | |
| mode: Joi.string().valid('relative', 'fixed'), | |
| exclude: Joi.func(), | |
| keepOnly: Joi.func(), | |
| include: Joi.array().items( | |
| MultiStageIncludeMemberFilterSchema, | |
| MultiStageIncludeConditionSchema | |
| ), | |
| }).nand('exclude', 'keepOnly'); |
| interval?: string; | ||
| type?: 'next' | 'prior'; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Nit: any type defeats type safety.
MultiStageFilterIncludeItem is typed as any, but you have a well-defined shape for it in the Joi schema (member, operator, values, and nested and/or groups). Consider giving it a structural type:
export type MultiStageFilterIncludeItem = {
member?: string;
operator?: string;
values?: string[];
and?: MultiStageFilterIncludeItem[];
or?: MultiStageFilterIncludeItem[];
};This would catch shape mismatches at compile time rather than at runtime in the Rust bridge.
| const MultiStageIncludeMemberFilterSchema = Joi.object().keys({ | ||
| member: Joi.alternatives(Joi.string(), Joi.func()).required(), | ||
| operator: Joi.any().valid( | ||
| 'equals', |
There was a problem hiding this comment.
Question: Is member: Joi.func() reachable?
The transpiler pattern (CubePropContextTranspiler.ts) only covers filter.exclude and filter.keepOnly — there's no pattern for filter.include[n].member. This means CUBE.dim references inside include items won't be transpiled, so the Joi.func() alternative for member can never be satisfied in practice for JavaScript models.
If the function form isn't intended to work here, removing it would avoid confusing schema authors who try member: CUBE.dim and get a runtime error. If it is intended, a transpiler pattern would need to be added.
| FilterItem::Segment(_) => {} | ||
| } |
There was a problem hiding this comment.
Nit: Segment items silently dropped in keep_only_members.
Both exclude_members and keep_only_members drop FilterItem::Segment entries. The old extract_filters_exclude_member also dropped segments, so this is pre-existing behavior. However, keep_only_members is new — if a user writes keepOnly: [orders.status] and the filter tree contains segments, they'll be silently removed even though the user didn't ask for that.
Not necessarily a bug (segments may not make sense in the keepOnly context), but worth a comment explaining the intent, or alternatively preserving segments unconditionally.
| /// Back-reference to the owning `QueryTools`. Set by `set_query_tools` | ||
| /// at the end of `QueryTools::try_new`, after the `Rc<QueryTools>` is | ||
| /// available. Held as `Weak` to avoid an `Rc` cycle: `QueryTools` owns | ||
| /// `Rc<RefCell<Compiler>>` strongly, so Compiler cannot also hold a | ||
| /// strong handle back without leaking. | ||
| query_tools: Weak<QueryTools>, | ||
| } |
There was a problem hiding this comment.
Positive: Good use of Weak to avoid Rc cycle.
The Weak<QueryTools> back-reference is the right pattern here — QueryTools owns Rc<RefCell<Compiler>>, so a strong Rc back would leak. The comment explaining why it's Weak is helpful for future maintainers.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10827 +/- ##
==========================================
- Coverage 78.86% 78.86% -0.01%
==========================================
Files 470 470
Lines 92289 92297 +8
Branches 3435 3438 +3
==========================================
+ Hits 72788 72794 +6
- Misses 18999 19001 +2
Partials 502 502
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @waralexrom's task in 4m 17s —— View job Claude finished reviewing this PR —— View job Review of PR #10827: Multi-stage filter directives (re-review after new commits)
Previous Review Feedback ResolutionThe two new commits since the initial review (
All four were done well. The Overall AssessmentThis is a clean, well-architected feature addition. The code quality is high across all layers. The Code QualityStrengths:
One minor observation:
SecurityNo concerns. The PerformanceNo concerns. Filter directive application happens once per multi-stage node during planning (not per-row). TestingTest coverage is excellent across all layers:
Edge cases covered: empty groups bubbling up, nested empty groups, One area that could benefit from additional testing (non-blocking):
Documentation
SummaryThis PR is in good shape. The previous review's actionable items have all been addressed, and the new segment handling is well-implemented with proper test coverage. No blocking issues remain. |
|
Claude finished @waralexrom's task in 3m 33s —— View job Review of PR #10827: Multi-stage filter directives (re-review on reopen)
Previous Review FeedbackAll four actionable items from earlier reviews have been resolved:
Overall AssessmentThis is a clean, well-architected feature. The Code QualityStrengths:
Minor observations (non-blocking):
SecurityNo concerns. The PerformanceNo concerns. Filter directive application happens once per multi-stage node during planning. TestingExcellent coverage across all layers:
Edge cases covered: empty groups bubbling up, nested empty groups, One area that could benefit from additional testing (non-blocking): The Documentation
SummaryThis PR is well-executed with no blocking issues. All prior review feedback has been addressed. The code is clean, well-tested, and the architecture makes the right trade-offs. The |
- Forbid `exclude` and `keepOnly` together at validation time (`.nand`), matching the Rust planner's mutual-exclusivity check — schema authors now get an early Joi error instead of a late planner error. - Drop `Joi.func()` from `filter.include[*].member`: the transpiler does not cover that path, so the function alternative is unreachable. - Replace `MultiStageFilterIncludeItem = any` with a structural union of member predicate / and-group / or-group, mirroring the Joi shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… values CubePropContextTranspiler does not cover filter.include[*].values, so a function form there would never receive CUBE/SECURITY_CONTEXT arguments and would fail at runtime — same reason Joi.func() was already removed from filter.include[*].member. Restrict the Joi schema and the TS type to plain arrays. Also reorder MultiStage* type declarations above DimensionDefinition so they are defined before use (no-use-before-define), and drop a stray blank line left over from the rebase in yaml-schema.test.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…yPlanner::try_new Move root_state construction out of plan_queries into a fallible constructor backed by a private build_root_state static. Drops the RefCell<Option<…>> + lazy-init + expect pattern in favor of an Rc<QueryProperties> field set once at construction time. plan_queries reuses self.root_state instead of rebuilding the same value locally. Also annotate the MultiStageFilter::include_time_dimension bucket and FilterCompiler::add_item with the pending time-dimension classification work: the bucket is structurally symmetric to QueryProperties but stays empty until FilterCompiler routes time-dim filters separately from plain dimensions. Drive-by: cargo fmt --all picks up a stale import order in tests/filter/tree_ops.rs left over from the feature commit; bundled in to keep the pre-commit hook green.
waralexrom
force-pushed
the
tesseract-set-of-multi-stage-directives
branch
from
474c3c2 to
3244476
Compare
|
Claude finished @waralexrom's task in 2m 53s —— View job Review of PR #10827: Multi-stage filter directives (re-review on reopen)
Previous Review FeedbackAll actionable items from earlier reviews have been addressed:
Overall AssessmentThis is a clean, well-architected feature addition. The Code QualityStrengths:
Minor observations (non-blocking):
SecurityNo concerns. The PerformanceNo concerns. Filter directive application happens once per multi-stage node during planning (not per-row). TestingExcellent coverage across all layers:
Edge cases covered: empty groups bubbling up, nested empty groups, One area that could benefit from additional testing (non-blocking): The Documentation
SummaryThis PR is well-executed with no blocking issues. All prior review feedback has been addressed. The code is clean, well-tested, and the architecture makes the right trade-offs. The |
…erage Adding `filter()` to MeasureDefinition / DimensionDefinition surfaces in the bridge harness as `invoke dispatcher out of sync` — extend both invoke fns to record the new field and add `filter` to the expected field lists for these two bridges. Register `multiStageFilter` itself as a first-class bridge in the harness: enable static-meta on `MultiStageFilterReferencesStatic` (derive NativeBridgeStatic + `with_static_meta` on the trait attribute), add the registry entry in bridge_test_exports.rs with an empty invoke fn (no trait methods — all data is serde-static, matching the filterGroup pattern), pin the expected field set ['exclude', 'include', 'keep_only', 'mode'], and ship a fully-populated fixture so serde is exercised end-to-end rather than just `try_new`.
2 participants
Summary
Adds a
filter:directive on multi-stage measures and dimensions that lets schema authors override the inherited filter context at the leaf state of a CTE. Supportsmode: relative|fixed,exclude,keepOnly, andinclude(with arbitrary AND/OR groups of query-style predicates).