WIP: FilterTree_Valid() validate predicate

RedisGraph · nafraf · Feb 18, 2023 · Feb 19, 2023 · Feb 20, 2023 · Feb 21, 2023
commit f2d41ed53b05c37ea31889ef92b79753f5cc9519
diff --git a/src/ast/ast_build_filter_tree.c b/src/ast/ast_build_filter_tree.c
@@ -322,7 +322,7 @@ FT_FilterNode *AST_BuildFilterTree(AST *ast) {
 		array_free(call_clauses);
 	}
 
-	if(!FilterTree_Valid(filter_tree)) {
+	if(!FilterTree_Valid(filter_tree, UINT8_MAX)) {
 		// Invalid filter tree structure, a compile-time error has been set.
 		FilterTree_Free(filter_tree);
 		return NULL;
@@ -361,7 +361,7 @@ FT_FilterNode *AST_BuildFilterTreeFromClauses
 		if(predicate) AST_ConvertFilters(&filter_tree, predicate);
 	}
 
-	if(!FilterTree_Valid(filter_tree)) {
+	if(!FilterTree_Valid(filter_tree, UINT8_MAX)) {
 		// Invalid filter tree structure, a compile-time error has been set.
 		FilterTree_Free(filter_tree);
 		return NULL;

diff --git a/src/execution_plan/execution_plan_build/build_pattern_comprehension_ops.c b/src/execution_plan/execution_plan_build/build_pattern_comprehension_ops.c
@@ -7,6 +7,7 @@
 #include "execution_plan_construct.h"
 #include "RG.h"
 #include "../ops/ops.h"
+#include "../../errors.h"
 #include "../../query_ctx.h"
 #include "../../util/rax_extensions.h"
 #include "../../ast/ast_build_filter_tree.h"
@@ -105,7 +106,7 @@ void buildPatternComprehensionOps
 			FT_FilterNode *filter_tree = NULL;
 			AST_ConvertFilters(&filter_tree, predicate);
 
-			if(!FilterTree_Valid(filter_tree)) {
+			if(!FilterTree_Valid(filter_tree, CYPHER_AST_PATTERN_COMPREHENSION)) {
 				// Invalid filter tree structure, a compile-time error has been set.
 				FilterTree_Free(filter_tree);
 			} else {

diff --git a/src/filter_tree/filter_tree.c b/src/filter_tree/filter_tree.c
@@ -666,7 +666,8 @@ static inline bool _FilterTree_ValidExpressionNode
 
 bool FilterTree_Valid
 (
-	const FT_FilterNode *root
+	const FT_FilterNode *root,
+	cypher_astnode_type_t type
 ) {
 	// An empty tree is has a valid structure.
 	if(!root) return true;
@@ -681,6 +682,19 @@ bool FilterTree_Valid
 				ErrorCtx_SetError("Filter predicate did not compare two expressions.");
 				return false;
 			}
+			// Aggregate functions can't be used as part of filters in pattern comprehension node
+			if (type == CYPHER_AST_PATTERN_COMPREHENSION) {
+				const char *func_name = AR_EXP_GetFuncName(root->pred.lhs);
+				if(AR_FuncIsAggregate(func_name)) {
+					ErrorCtx_SetError("Invalid use of aggregating function '%s' in pattern comprehension predicate", func_name);
+					return false;
+				}
+				func_name = AR_EXP_GetFuncName(root->pred.rhs);
+				if(AR_FuncIsAggregate(func_name)) {
+					ErrorCtx_SetError("Invalid use of aggregating function '%s' in pattern comprehension predicate", func_name);
+					return false;
+				}
+			}
 			break;
 		case FT_N_COND:
 			// Empty condition, invalid structure.
@@ -694,8 +708,8 @@ bool FilterTree_Valid
 				ErrorCtx_SetError("Invalid usage of 'NOT' filter.");
 				return false;
 			}
-			if(!FilterTree_Valid(root->cond.left)) return false;
-			if(!FilterTree_Valid(root->cond.right)) return false;
+			if(!FilterTree_Valid(root->cond.left, type)) return false;
+			if(!FilterTree_Valid(root->cond.right, type)) return false;
 			break;
 		default:
 			ASSERT("Unknown filter tree node" && false);

diff --git a/src/filter_tree/filter_tree.h b/src/filter_tree/filter_tree.h
@@ -181,7 +181,8 @@ FT_FilterNode *FilterTree_Combine
 // a condition or predicate node can't be childless
 bool FilterTree_Valid
 (
-	const FT_FilterNode *root
+	const FT_FilterNode *root,
+	cypher_astnode_type_t type
 );
 
 // remove NOT nodes by applying DeMorgan laws

diff --git a/tests/flow/test_comprehension_functions.py b/tests/flow/test_comprehension_functions.py
@@ -41,6 +41,13 @@ def populate_graph(self):
 
         redis_graph.commit()
 
+    def expect_error(self, query, expected_err_msg):
+        try:
+            redis_graph.query(query)
+            assert(False)
+        except redis.exceptions.ResponseError as e:
+            self.env.assertIn(expected_err_msg, str(e))
+
     # Test list comprehension queries with scalar inputs and a single result row
     def test01_list_comprehension_single_return(self):
         expected_result = [[[2, 6]]]
@@ -462,3 +469,12 @@ def test20_pattern_comprehension_in_switch_case(self):
         expected_result = [[[1, 1]], [[2]], [[3]], [[]]]
         self.env.assertEquals(actual_result.result_set, expected_result)
 
+    def test21_pattern_comprehension_with_invalid_filters(self):
+        # A list of queries and errors which are expected to occur with the specified query.
+        queries_with_errors = {
+            "MATCH (x) RETURN [(x)--(z) WHERE collect(z.a) > 10 | z.b]": "Invalid use of aggregating function 'collect' in pattern comprehension predicate",
+            "MATCH (x) RETURN [(x)--(z) WHERE z.a > collect(10) | z.b]": "Invalid use of aggregating function 'collect' in pattern comprehension predicate",
+            "MATCH (x) RETURN [(x)--(z) WHERE z.a > 10 | collect(z.b)]": "Invalid use of aggregating function 'collect'",
+        }
+        for query, error in queries_with_errors.items():
+            self.expect_error(query, error)