MQE: separate feature flag for binary comparison operations (#9504) (#…

…9506) * MQE: separate feature flag for binary comparison operations * Add changelog entry (cherry picked from commit 204d110) Co-authored-by: Charles Korn <[email protected]>
grafana · Oct 3, 2024 · a47f97d · a47f97d
1 parent e8ae03e
commit a47f97d
Show file tree

Hide file tree

Showing 7 changed files with 106 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -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`

diff --git a/cmd/mimir/config-descriptor.json b/cmd/mimir/config-descriptor.json
@@ -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"
             },

diff --git a/cmd/mimir/help-all.txt.tmpl b/cmd/mimir/help-all.txt.tmpl
@@ -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

diff --git a/docs/sources/mimir/configure/configuration-parameters/index.md b/docs/sources/mimir/configure/configuration-parameters/index.md
@@ -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.

diff --git a/pkg/streamingpromql/config.go b/pkg/streamingpromql/config.go
@@ -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.
@@ -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.")
 }
diff --git a/pkg/streamingpromql/engine_test.go b/pkg/streamingpromql/engine_test.go
@@ -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{}")

diff --git a/pkg/streamingpromql/query.go b/pkg/streamingpromql/query.go
@@ -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
@@ -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
@@ -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))
@@ -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)