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
12 changes: 12 additions & 0 deletions packages/cubejs-backend-native/src/bridge_test_exports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ use cubesqlplanner::cube_bridge::{
multi_stage_filter::{
multi_stage_filter_references_bridge_fields_meta, NativeMultiStageFilterReferences,
},
multi_stage_grain::{
multi_stage_grain_references_bridge_fields_meta, NativeMultiStageGrainReferences,
},
pre_aggregation_description::{
pre_aggregation_description_bridge_fields_meta, NativePreAggregationDescription,
PreAggregationDescription,
Expand Down Expand Up @@ -450,6 +453,7 @@ bridge_registry! {
"memberExpressionDefinition" => NativeMemberExpressionDefinition, member_expression_definition_bridge_fields_meta, invoke_member_expression_definition;
"memberOrderBy" => NativeMemberOrderBy, member_order_by_bridge_fields_meta, invoke_member_order_by;
"multiStageFilter" => NativeMultiStageFilterReferences, multi_stage_filter_references_bridge_fields_meta, invoke_multi_stage_filter;
"multiStageGrain" => NativeMultiStageGrainReferences, multi_stage_grain_references_bridge_fields_meta, invoke_multi_stage_grain;
"preAggregationDescription" => NativePreAggregationDescription, pre_aggregation_description_bridge_fields_meta, invoke_pre_aggregation_description;
"preAggregationObj" => NativePreAggregationObj, pre_aggregation_obj_bridge_fields_meta, invoke_pre_aggregation_obj;
"preAggregationTimeDimension" => NativePreAggregationTimeDimension, pre_aggregation_time_dimension_bridge_fields_meta, invoke_pre_aggregation_time_dimension;
Expand Down Expand Up @@ -748,6 +752,7 @@ fn invoke_measure_definition<IT: InnerTypes>(b: &NativeMeasureDefinition<IT>) ->
r.record("case", b.case());
r.record("filters", b.filters());
r.record("filter", b.filter());
r.record("grain", b.grain());
r.record("drill_filters", b.drill_filters());
r.record("order_by", b.order_by());
r.record("mask_sql", b.mask_sql());
Expand All @@ -764,6 +769,13 @@ fn invoke_multi_stage_filter<IT: InnerTypes>(
InvokeResult::new()
}

fn invoke_multi_stage_grain<IT: InnerTypes>(
_b: &NativeMultiStageGrainReferences<IT>,
) -> InvokeResult {
// Static-only bridge — same shape as `invoke_multi_stage_filter`.
InvokeResult::new()
}

fn invoke_expression_struct<IT: InnerTypes>(b: &NativeExpressionStruct<IT>) -> InvokeResult {
let mut r = InvokeResult::new();
r.record("add_filters", b.add_filters());
Expand Down
12 changes: 12 additions & 0 deletions packages/cubejs-backend-native/test/bridge/bridge-fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,17 @@ export const multiStageFilterFixture = (): unknown => ({
include: [{ member: 'orders.amount', operator: 'gt', values: ['0'] }],
});

// MultiStageGrainReferences mirrors the filter bridge — static-only. The
// bridge is structurally permissive (both `excludeReferences` and
// `keepOnlyReferences` could deserialize at once); the `.nand` lives on the
// JS Joi validator. The fixture populates only one of them to match the
// schema contract. `include` is a plain reference list, not the structured
// filter items the filter bridge uses.
export const multiStageGrainFixture = (): unknown => ({
excludeReferences: ['orders.region'],
includeReferences: ['orders.category'],
});

export const memberDefinitionFixture = (): unknown => ({
type: 'dimension',
// sql is optional
Expand Down Expand Up @@ -284,6 +295,7 @@ export const FIXTURES: Record<string, BridgeFixtureFactory> = {
memberExpressionDefinition: memberExpressionDefinitionFixture,
memberOrderBy: memberOrderByFixture,
multiStageFilter: multiStageFilterFixture,
multiStageGrain: multiStageGrainFixture,
preAggregationDescription: preAggregationDescriptionFixture,
preAggregationObj: preAggregationObjFixture,
preAggregationTimeDimension: preAggregationTimeDimensionFixture,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ const BRIDGES: BridgeSpec[] = [
'drill_filters',
'filter',
'filters',
'grain',
'group_by_references',
'mask_sql',
'measure_type',
Expand All @@ -203,6 +204,10 @@ const BRIDGES: BridgeSpec[] = [
name: 'multiStageFilter',
expected: ['exclude', 'include', 'keep_only', 'mode'],
},
{
name: 'multiStageGrain',
expected: ['exclude', 'include', 'keep_only'],
},
{
name: 'preAggregationDescription',
expected: [
Expand Down
22 changes: 22 additions & 0 deletions packages/cubejs-schema-compiler/src/compiler/CubeEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,16 @@ export type MultiStageFilterDirective = {
keepOnlyReferences?: string[];
};

export type MultiStageGrainDirective = {
exclude?: (...args: Array<unknown>) => Array<ToString>;
keepOnly?: (...args: Array<unknown>) => Array<ToString>;
include?: (...args: Array<unknown>) => Array<ToString>;
// Resolved sibling fields populated by `evaluateMultiStageReferences`.
excludeReferences?: string[];
keepOnlyReferences?: string[];
includeReferences?: string[];
};

export type DimensionDefinition = {
type: string;
sql(): string;
Expand Down Expand Up @@ -98,6 +108,7 @@ export type MeasureDefinition = {
groupBy?: (...args: Array<unknown>) => Array<ToString>;
reduceBy?: (...args: Array<unknown>) => Array<ToString>;
addGroupBy?: (...args: Array<unknown>) => Array<ToString>;
grain?: MultiStageGrainDirective;
timeShift?: TimeShiftDefinition[];
groupByReferences?: string[];
reduceByReferences?: string[];
Expand Down Expand Up @@ -626,6 +637,17 @@ export class CubeEvaluator extends CubeSymbols {
member.filter.keepOnlyReferences = this.evaluateReferences(cubeName, member.filter.keepOnly);
}
}
if (member.grain) {
if (typeof member.grain.exclude === 'function') {
member.grain.excludeReferences = this.evaluateReferences(cubeName, member.grain.exclude);
}
if (typeof member.grain.keepOnly === 'function') {
member.grain.keepOnlyReferences = this.evaluateReferences(cubeName, member.grain.keepOnly);
}
if (typeof member.grain.include === 'function') {
member.grain.includeReferences = this.evaluateReferences(cubeName, member.grain.include);
}
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,12 @@ const MultiStageFilter = Joi.object().keys({
),
}).nand('exclude', 'keepOnly');

const MultiStageGrain = Joi.object().keys({
exclude: Joi.func(),
keepOnly: Joi.func(),
include: Joi.func(),
}).nand('exclude', 'keepOnly');
Comment on lines +871 to +875

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.

The Joi MultiStageGrain validator uses .nand('exclude', 'keepOnly') but doesn't enforce that at least one of the three fields (exclude, keepOnly, include) is present. An empty grain: {} would pass validation and silently become a no-op MultiStageGrain::default() on the Rust side.

This is consistent with how MultiStageFilter works (it also allows an empty object), so it may be intentional. But unlike filter: (where an empty object has a meaningful "no filter overrides" reading), an empty grain: {} on a multi-stage measure is almost certainly a user mistake. Consider adding .or('exclude', 'keepOnly', 'include') to catch this early:

Suggested change
const MultiStageGrain = Joi.object().keys({
exclude: Joi.func(),
keepOnly: Joi.func(),
include: Joi.func(),
}).nand('exclude', 'keepOnly');
const MultiStageGrain = Joi.object().keys({
exclude: Joi.func(),
keepOnly: Joi.func(),
include: Joi.func(),
}).nand('exclude', 'keepOnly').or('exclude', 'keepOnly', 'include');


const CaseSchema = Joi.object().keys({
when: Joi.array().items(Joi.object().keys({
sql: Joi.func().required(),
Expand Down Expand Up @@ -916,6 +922,7 @@ const MeasuresSchema = Joi.object().pattern(identifierRegex, Joi.alternatives().
reduceBy: Joi.func(),
addGroupBy: Joi.func(),
filter: MultiStageFilter,
grain: MultiStageGrain,
timeShift: Joi.alternatives().conditional(Joi.array().length(1), {
then: Joi.array().items(timeShiftItemOptional),
otherwise: Joi.array().items(timeShiftItemRequired)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const transpiledFieldsPatterns: Array<RegExp> = [
/^measures\.[_a-zA-Z][_a-zA-Z0-9]*\.(timeShift|time_shift)\.[0-9]+\.(timeDimension|time_dimension)$/,
/^measures\.[_a-zA-Z][_a-zA-Z0-9]*\.(reduceBy|reduce_by|groupBy|group_by|addGroupBy|add_group_by)$/,
/^(measures|dimensions)\.[_a-zA-Z][_a-zA-Z0-9]*\.filter\.(exclude|keepOnly|keep_only)$/,
/^measures\.[_a-zA-Z][_a-zA-Z0-9]*\.grain\.(exclude|keepOnly|keep_only|include)$/,

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.

Minor: The transpiler pattern supports keep_only as a snake_case alias but doesn't include an exclude snake_case variant (it's already snake_case). Worth noting that include also doesn't need a snake_case alias since it's already all lowercase. This looks correct as-is.

However, I notice the filter pattern on line 22 supports (measures|dimensions) but the grain pattern is restricted to measures only. This is consistent with the Joi validator (which only adds grain to MeasuresSchema, not dimensions). Good — just flagging for visibility that this deliberate asymmetry between filter and grain scope is correctly threaded through all three layers (Joi, transpiler, Rust bridge).

/^(measures|dimensions)\.[_a-zA-Z][_a-zA-Z0-9]*\.case\.switch$/,
/^dimensions\.[_a-zA-Z][_a-zA-Z0-9]*\.(reduceBy|reduce_by|groupBy|group_by|addGroupBy|add_group_by|key)$/,
/^(preAggregations|pre_aggregations)\.[_a-zA-Z][_a-zA-Z0-9]*\.indexes\.[_a-zA-Z][_a-zA-Z0-9]*\.columns$/,
Expand Down
Loading
Loading