Skip to content
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.

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

[r310] MQE: separate feature flag for binary comparison operations #9506

Merged
merged 1 commit into from
Oct 3, 2024
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* `cortex_alertmanager_alerts`
* `cortex_alertmanager_silences`
* [CHANGE] Cache: Deprecate experimental support for Redis as a cache backend. #9453
* [FEATURE] Querier: add experimental streaming PromQL engine, enabled with `-querier.query-engine=mimir`. #9367 #9368 #9398 #9399 #9403 #9417 #9418 #9419 #9420 #9482
* [FEATURE] Querier: add experimental streaming PromQL engine, enabled with `-querier.query-engine=mimir`. #9367 #9368 #9398 #9399 #9403 #9417 #9418 #9419 #9420 #9482 #9504
* [FEATURE] Query-frontend: added experimental configuration options `query-frontend.cache-errors` and `query-frontend.results-cache-ttl-for-errors` to allow non-transient responses to be cached. When set to `true` error responses from hitting limits or bad data are cached for a short TTL. #9028
* [FEATURE] gRPC: Support S2 compression. #9322
* `-alertmanager.alertmanager-client.grpc-compression=s2`
Expand Down
28 changes: 25 additions & 3 deletions cmd/mimir/config-descriptor.json
Original file line number Diff line number Diff line change
Expand Up @@ -1980,12 +1980,34 @@
},
{
"kind": "field",
"name": "enable_binary_comparison_operations",
"name": "enable_vector_vector_binary_comparison_operations",
"required": false,
"desc": "Enable support for binary comparison operations in Mimir's query engine. Only applies if the Mimir query engine is in use.",
"desc": "Enable support for binary comparison operations between two vectors in Mimir's query engine. Only applies if the Mimir query engine is in use.",
"fieldValue": null,
"fieldDefaultValue": true,
"fieldFlag": "querier.mimir-query-engine.enable-binary-comparison-operations",
"fieldFlag": "querier.mimir-query-engine.enable-vector-vector-binary-comparison-operations",
"fieldType": "boolean",
"fieldCategory": "experimental"
},
{
"kind": "field",
"name": "enable_vector_scalar_binary_comparison_operations",
"required": false,
"desc": "Enable support for binary comparison operations between a vector and a scalar in Mimir's query engine. Only applies if the Mimir query engine is in use.",
"fieldValue": null,
"fieldDefaultValue": true,
"fieldFlag": "querier.mimir-query-engine.enable-vector-scalar-binary-comparison-operations",
"fieldType": "boolean",
"fieldCategory": "experimental"
},
{
"kind": "field",
"name": "enable_scalar_scalar_binary_comparison_operations",
"required": false,
"desc": "Enable support for binary comparison operations between two scalars in Mimir's query engine. Only applies if the Mimir query engine is in use.",
"fieldValue": null,
"fieldDefaultValue": true,
"fieldFlag": "querier.mimir-query-engine.enable-scalar-scalar-binary-comparison-operations",
"fieldType": "boolean",
"fieldCategory": "experimental"
},
Expand Down
8 changes: 6 additions & 2 deletions cmd/mimir/help-all.txt.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -1937,10 +1937,14 @@ Usage of ./cmd/mimir/mimir:
Maximum number of samples a single query can load into memory. This config option should be set on query-frontend too when query sharding is enabled. (default 50000000)
-querier.mimir-query-engine.enable-aggregation-operations
[experimental] Enable support for aggregation operations in Mimir's query engine. Only applies if the Mimir query engine is in use. (default true)
-querier.mimir-query-engine.enable-binary-comparison-operations
[experimental] Enable support for binary comparison operations in Mimir's query engine. Only applies if the Mimir query engine is in use. (default true)
-querier.mimir-query-engine.enable-scalar-scalar-binary-comparison-operations
[experimental] Enable support for binary comparison operations between two scalars in Mimir's query engine. Only applies if the Mimir query engine is in use. (default true)
-querier.mimir-query-engine.enable-scalars
[experimental] Enable support for scalars in Mimir's query engine. Only applies if the Mimir query engine is in use. (default true)
-querier.mimir-query-engine.enable-vector-scalar-binary-comparison-operations
[experimental] Enable support for binary comparison operations between a vector and a scalar in Mimir's query engine. Only applies if the Mimir query engine is in use. (default true)
-querier.mimir-query-engine.enable-vector-vector-binary-comparison-operations
[experimental] Enable support for binary comparison operations between two vectors in Mimir's query engine. Only applies if the Mimir query engine is in use. (default true)
-querier.minimize-ingester-requests
If true, when querying ingesters, only the minimum required ingesters required to reach quorum will be queried initially, with other ingesters queried only if needed due to failures from the initial set of ingesters. Enabling this option reduces resource consumption for the happy path at the cost of increased latency for the unhappy path. (default true)
-querier.minimize-ingester-requests-hedging-delay duration
Expand Down
21 changes: 17 additions & 4 deletions docs/sources/mimir/configure/configuration-parameters/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -1504,10 +1504,23 @@ mimir_query_engine:
# CLI flag: -querier.mimir-query-engine.enable-aggregation-operations
[enable_aggregation_operations: <boolean> | default = true]
# (experimental) Enable support for binary comparison operations in Mimir's
# query engine. Only applies if the Mimir query engine is in use.
# CLI flag: -querier.mimir-query-engine.enable-binary-comparison-operations
[enable_binary_comparison_operations: <boolean> | default = true]
# (experimental) Enable support for binary comparison operations between two
# vectors in Mimir's query engine. Only applies if the Mimir query engine is
# in use.
# CLI flag: -querier.mimir-query-engine.enable-vector-vector-binary-comparison-operations
[enable_vector_vector_binary_comparison_operations: <boolean> | default = true]
# (experimental) Enable support for binary comparison operations between a
# vector and a scalar in Mimir's query engine. Only applies if the Mimir query
# engine is in use.
# CLI flag: -querier.mimir-query-engine.enable-vector-scalar-binary-comparison-operations
[enable_vector_scalar_binary_comparison_operations: <boolean> | default = true]
# (experimental) Enable support for binary comparison operations between two
# scalars in Mimir's query engine. Only applies if the Mimir query engine is
# in use.
# CLI flag: -querier.mimir-query-engine.enable-scalar-scalar-binary-comparison-operations
[enable_scalar_scalar_binary_comparison_operations: <boolean> | default = true]
# (experimental) Enable support for scalars in Mimir's query engine. Only
# applies if the Mimir query engine is in use.
Expand Down
14 changes: 10 additions & 4 deletions pkg/streamingpromql/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ type EngineOpts struct {
}

type FeatureToggles struct {
EnableAggregationOperations bool `yaml:"enable_aggregation_operations" category:"experimental"`
EnableBinaryComparisonOperations bool `yaml:"enable_binary_comparison_operations" category:"experimental"`
EnableScalars bool `yaml:"enable_scalars" category:"experimental"`
EnableAggregationOperations bool `yaml:"enable_aggregation_operations" category:"experimental"`
EnableVectorVectorBinaryComparisonOperations bool `yaml:"enable_vector_vector_binary_comparison_operations" category:"experimental"`
EnableVectorScalarBinaryComparisonOperations bool `yaml:"enable_vector_scalar_binary_comparison_operations" category:"experimental"`
EnableScalarScalarBinaryComparisonOperations bool `yaml:"enable_scalar_scalar_binary_comparison_operations" category:"experimental"`
EnableScalars bool `yaml:"enable_scalars" category:"experimental"`
}

// EnableAllFeatures enables all features supported by MQE, including experimental or incomplete features.
Expand All @@ -29,10 +31,14 @@ var EnableAllFeatures = FeatureToggles{
true,
true,
true,
true,
true,
}

func (t *FeatureToggles) RegisterFlags(f *flag.FlagSet) {
f.BoolVar(&t.EnableAggregationOperations, "querier.mimir-query-engine.enable-aggregation-operations", true, "Enable support for aggregation operations in Mimir's query engine. Only applies if the Mimir query engine is in use.")
f.BoolVar(&t.EnableBinaryComparisonOperations, "querier.mimir-query-engine.enable-binary-comparison-operations", true, "Enable support for binary comparison operations in Mimir's query engine. Only applies if the Mimir query engine is in use.")
f.BoolVar(&t.EnableVectorVectorBinaryComparisonOperations, "querier.mimir-query-engine.enable-vector-vector-binary-comparison-operations", true, "Enable support for binary comparison operations between two vectors in Mimir's query engine. Only applies if the Mimir query engine is in use.")
f.BoolVar(&t.EnableVectorScalarBinaryComparisonOperations, "querier.mimir-query-engine.enable-vector-scalar-binary-comparison-operations", true, "Enable support for binary comparison operations between a vector and a scalar in Mimir's query engine. Only applies if the Mimir query engine is in use.")
f.BoolVar(&t.EnableScalarScalarBinaryComparisonOperations, "querier.mimir-query-engine.enable-scalar-scalar-binary-comparison-operations", true, "Enable support for binary comparison operations between two scalars in Mimir's query engine. Only applies if the Mimir query engine is in use.")
f.BoolVar(&t.EnableScalars, "querier.mimir-query-engine.enable-scalars", true, "Enable support for scalars in Mimir's query engine. Only applies if the Mimir query engine is in use.")
}
44 changes: 38 additions & 6 deletions pkg/streamingpromql/engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,48 @@ func TestUnsupportedPromQLFeaturesWithFeatureToggles(t *testing.T) {
requireQueryIsUnsupported(t, featureToggles, "sum by (label) (metric)", "aggregation operations")
})

t.Run("binary expressions with comparison operation", func(t *testing.T) {
t.Run("vector/vector binary expressions with comparison operation", func(t *testing.T) {
featureToggles := EnableAllFeatures
featureToggles.EnableBinaryComparisonOperations = false
featureToggles.EnableVectorVectorBinaryComparisonOperations = false

requireQueryIsUnsupported(t, featureToggles, "metric{} > other_metric{}", "binary expression with '>'")
requireQueryIsUnsupported(t, featureToggles, "metric{} > 1", "binary expression with '>'")
requireQueryIsUnsupported(t, featureToggles, "1 > metric{}", "binary expression with '>'")
requireQueryIsUnsupported(t, featureToggles, "2 > bool 1", "binary expression with '>'")
requireQueryIsUnsupported(t, featureToggles, "metric{} > other_metric{}", "vector/vector binary expression with '>'")

// Other operations should still be supported.
requireQueryIsSupported(t, featureToggles, "metric{} > 1")
requireQueryIsSupported(t, featureToggles, "1 > metric{}")
requireQueryIsSupported(t, featureToggles, "2 > bool 1")
requireQueryIsSupported(t, featureToggles, "metric{} + other_metric{}")
requireQueryIsSupported(t, featureToggles, "metric{} + 1")
requireQueryIsSupported(t, featureToggles, "1 + metric{}")
requireQueryIsSupported(t, featureToggles, "2 + 1")
})

t.Run("vector/scalar binary expressions with comparison operation", func(t *testing.T) {
featureToggles := EnableAllFeatures
featureToggles.EnableVectorScalarBinaryComparisonOperations = false

requireQueryIsUnsupported(t, featureToggles, "metric{} > 1", "vector/scalar binary expression with '>'")
requireQueryIsUnsupported(t, featureToggles, "1 > metric{}", "vector/scalar binary expression with '>'")

// Other operations should still be supported.
requireQueryIsSupported(t, featureToggles, "metric{} > other_metric{}")
requireQueryIsSupported(t, featureToggles, "2 > bool 1")
requireQueryIsSupported(t, featureToggles, "metric{} + other_metric{}")
requireQueryIsSupported(t, featureToggles, "metric{} + 1")
requireQueryIsSupported(t, featureToggles, "1 + metric{}")
requireQueryIsSupported(t, featureToggles, "2 + 1")
})

t.Run("scalar/scalar binary expressions with comparison operation", func(t *testing.T) {
featureToggles := EnableAllFeatures
featureToggles.EnableScalarScalarBinaryComparisonOperations = false

requireQueryIsUnsupported(t, featureToggles, "2 > bool 1", "scalar/scalar binary expression with '>'")

// Other operations should still be supported.
requireQueryIsSupported(t, featureToggles, "metric{} > other_metric{}")
requireQueryIsSupported(t, featureToggles, "metric{} > 1")
requireQueryIsSupported(t, featureToggles, "1 > metric{}")
requireQueryIsSupported(t, featureToggles, "metric{} + other_metric{}")
requireQueryIsSupported(t, featureToggles, "metric{} + 1")
requireQueryIsSupported(t, featureToggles, "1 + metric{}")
Expand Down
15 changes: 9 additions & 6 deletions pkg/streamingpromql/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,6 @@ func (q *Query) convertToInstantVectorOperator(expr parser.Expr) (types.InstantV
case *parser.Call:
return q.convertFunctionCallToInstantVectorOperator(e)
case *parser.BinaryExpr:
if e.Op.IsComparisonOperator() && !q.engine.featureToggles.EnableBinaryComparisonOperations {
return nil, compat.NewNotSupportedError(fmt.Sprintf("binary expression with '%v'", e.Op))
}

// We only need to handle three combinations of types here:
// Scalar on left, vector on right
// Vector on left, scalar on right
Expand All @@ -177,6 +173,10 @@ func (q *Query) convertToInstantVectorOperator(expr parser.Expr) (types.InstantV
// We don't need to handle scalars on both sides here, as that would produce a scalar and so is handled in convertToScalarOperator.

if e.LHS.Type() == parser.ValueTypeScalar || e.RHS.Type() == parser.ValueTypeScalar {
if e.Op.IsComparisonOperator() && !q.engine.featureToggles.EnableVectorScalarBinaryComparisonOperations {
return nil, compat.NewNotSupportedError(fmt.Sprintf("vector/scalar binary expression with '%v'", e.Op))
}

var scalar types.ScalarOperator
var vector types.InstantVectorOperator
var err error
Expand Down Expand Up @@ -214,6 +214,9 @@ func (q *Query) convertToInstantVectorOperator(expr parser.Expr) (types.InstantV
}

// Vectors on both sides.
if e.Op.IsComparisonOperator() && !q.engine.featureToggles.EnableVectorVectorBinaryComparisonOperations {
return nil, compat.NewNotSupportedError(fmt.Sprintf("vector/vector binary expression with '%v'", e.Op))
}

if e.VectorMatching.Card != parser.CardOneToOne {
return nil, compat.NewNotSupportedError(fmt.Sprintf("binary expression with %v matching", e.VectorMatching.Card))
Expand Down Expand Up @@ -342,8 +345,8 @@ func (q *Query) convertToScalarOperator(expr parser.Expr) (types.ScalarOperator,
case *parser.ParenExpr:
return q.convertToScalarOperator(e.Expr)
case *parser.BinaryExpr:
if e.Op.IsComparisonOperator() && !q.engine.featureToggles.EnableBinaryComparisonOperations {
return nil, compat.NewNotSupportedError(fmt.Sprintf("binary expression with '%v'", e.Op))
if e.Op.IsComparisonOperator() && !q.engine.featureToggles.EnableScalarScalarBinaryComparisonOperations {
return nil, compat.NewNotSupportedError(fmt.Sprintf("scalar/scalar binary expression with '%v'", e.Op))
}

lhs, err := q.convertToScalarOperator(e.LHS)
Expand Down
Loading