added params support to id seek (#1164) (#1218)

* added params support to id seek

* reduce to scalar with runtime

* fixed PR comments

* fixed PR comments

(cherry picked from commit bc609c3)

src/arithmetic/arithmetic_expression.c

@@ -172,10 +172,18 @@ AR_ExpNode *AR_EXP_NewParameterOperandNode(const char *param_name) {
  * e.g. MINUS(X) where X is a constant number will be reduced to
  * a single node with the value -X
  * PLUS(MINUS(A), B) will be reduced to a single constant: B-A. */
-bool AR_EXP_ReduceToScalar(AR_ExpNode *root) {
+bool AR_EXP_ReduceToScalar(AR_ExpNode *root, bool reduce_params, SIValue *val) {
+	if(val != NULL) *val = SI_NullVal();
 	if(root->type == AR_EXP_OPERAND) {
-		if(root->operand.type == AR_EXP_CONSTANT) {
+		// In runtime, parameters are set so they can be evaluated
+		if(reduce_params && AR_EXP_IsParameter(root)) {
+			SIValue v = AR_EXP_Evaluate(root, NULL);
+			if(val != NULL) *val = v;
+			return true;
+		}
+		if(AR_EXP_IsConstant(root)) {
 			// Root is already a constant
+			if(val != NULL) *val = root->operand.constant;
 			return true;
 		}
 		// Root is variadic, no way to reduce.
@@ -189,7 +197,7 @@ bool AR_EXP_ReduceToScalar(AR_ExpNode *root) {
 			 * if so we'll be able to reduce root. */
 			bool reduce_children = true;
 			for(int i = 0; i < root->op.child_count; i++) {
-				if(!AR_EXP_ReduceToScalar(root->op.children[i])) {
+				if(!AR_EXP_ReduceToScalar(root->op.children[i], reduce_params, NULL)) {
 					// Root reduce is not possible, but continue to reduce every reducable child.
 					reduce_children = false;
 				}
@@ -204,6 +212,7 @@ bool AR_EXP_ReduceToScalar(AR_ExpNode *root) {
 
 			// Evaluate function.
 			SIValue v = AR_EXP_Evaluate(root, NULL);
+			if(val != NULL) *val = v;
 			if(SIValue_IsNull(v)) return false;
 
 			// Reduce.
@@ -447,7 +456,7 @@ SIValue AR_EXP_Evaluate(AR_ExpNode *root, const Record r) {
 	}
 	// At least one param node was encountered during evaluation, tree should be param node free.
 	// Try reducing the tree.
-	if(res == EVAL_FOUND_PARAM) AR_EXP_ReduceToScalar(root);
+	if(res == EVAL_FOUND_PARAM) AR_EXP_ReduceToScalar(root, true, NULL);
 	return result;
 }
 

src/arithmetic/arithmetic_expression.h

@@ -107,8 +107,11 @@ AR_ExpNode *AR_EXP_NewParameterOperandNode(const char *param_name);
 /* Returns if the operation is distinct aggregation */
 bool AR_EXP_PerformDistinct(AR_ExpNode *op);
 
-/* Compact tree by evaluating all contained functions that can be resolved right now. */
-bool AR_EXP_ReduceToScalar(AR_ExpNode *root);
+/* Compact tree by evaluating all contained functions that can be resolved right now.
+ * The function returns true if it managed to compact the expression.
+ * The reduce_params flag indicates if parameters should be evaluated.
+ * The val pointer is out-by-ref returned computation. */
+bool AR_EXP_ReduceToScalar(AR_ExpNode *root, bool reduce_params, SIValue *val);
 
 /* Evaluate arithmetic expression tree. */
 SIValue AR_EXP_Evaluate(AR_ExpNode *root, const Record r);

src/ast/ast_build_ar_exp.c

@@ -456,7 +456,7 @@ static AR_ExpNode *_AR_EXP_FromExpression(const cypher_astnode_t *expr) {
 
 AR_ExpNode *AR_EXP_FromExpression(const cypher_astnode_t *expr) {
 	AR_ExpNode *root = _AR_EXP_FromExpression(expr);
-	AR_EXP_ReduceToScalar(root);
+	AR_EXP_ReduceToScalar(root, false, NULL);
 
 	/* Make sure expression doesn't contains nested aggregation functions
 	 * count(max(n.v)) */

src/execution_plan/optimizations/seek_by_id.c

@@ -19,7 +19,7 @@ static bool _idFilter(FT_FilterNode *f, AST_Operator *rel, EntityID *id, bool *r
 	if(f->pred.op == OP_NEQUAL) return false;
 
 	AR_OpNode *op;
-	AR_OperandNode *operand;
+	AR_ExpNode *expr;
 	AR_ExpNode *lhs = f->pred.lhs;
 	AR_ExpNode *rhs = f->pred.rhs;
 	*rel = f->pred.op;
@@ -29,20 +29,22 @@ static bool _idFilter(FT_FilterNode *f, AST_Operator *rel, EntityID *id, bool *r
 	 * const compare ID(N) */
 	if(lhs->type == AR_EXP_OPERAND && rhs->type == AR_EXP_OP) {
 		op = &rhs->op;
-		operand = &lhs->operand;
+		expr = lhs;
 		*reverse = true;
 	} else if(lhs->type == AR_EXP_OP && rhs->type == AR_EXP_OPERAND) {
 		op = &lhs->op;
-		operand = &rhs->operand;
+		expr = rhs;
 		*reverse = false;
 	} else {
 		return false;
 	}
 
-	// Make sure ID is compared to a constant.
-	if(operand->type != AR_EXP_CONSTANT) return false;
-	if(SI_TYPE(operand->constant) != T_INT64) return false;
-	*id = SI_GET_NUMERIC(operand->constant);
+	// Make sure ID is compared to a constant int64.
+	SIValue val;
+	bool reduced = AR_EXP_ReduceToScalar(expr, true, &val);
+	if(!reduced) return false;
+	if(SI_TYPE(val) != T_INT64) return false;
+	*id = SI_GET_NUMERIC(val);
 
 	// Make sure applied function is ID.
 	if(strcasecmp(op->func_name, "id")) return false;

src/execution_plan/optimizations/utilize_indices.c

@@ -218,11 +218,10 @@ static bool _validateInExpression(AR_ExpNode *exp) {
 	assert(exp->op.child_count == 2);
 
 	AR_ExpNode *list = exp->op.children[1];
-	// In case the array is a parameter such as "WHERE x IN $arr", evaluate the parameter for in-place replacement.
-	if(AR_EXP_IsParameter(list)) AR_EXP_Evaluate(list, NULL);
-	if(list->operand.type != AR_EXP_CONSTANT || list->operand.constant.type != T_ARRAY) return false;
+	SIValue listValue = SI_NullVal();
+	AR_EXP_ReduceToScalar(list, true, &listValue);
+	if(SI_TYPE(listValue) != T_ARRAY) return false;
 
-	SIValue listValue = list->operand.constant;
 	uint listLen = SIArray_Length(listValue);
 	for(uint i = 0; i < listLen; i++) {
 		SIValue v = SIArray_Get(listValue, i);
@@ -242,20 +241,15 @@ bool _simple_predicates(const FT_FilterNode *filter) {
 	case FT_N_PRED:
 		if(filter->pred.rhs->type == AR_EXP_OPERAND &&
 		   filter->pred.lhs->type == AR_EXP_OPERAND) {
-			// In case of parameters, evalation will transform them into constants (in-place replacement).
-			if(AR_EXP_IsParameter(filter->pred.lhs)) AR_EXP_Evaluate(filter->pred.lhs, NULL);
-			if(AR_EXP_IsParameter(filter->pred.rhs)) AR_EXP_Evaluate(filter->pred.rhs, NULL);
-			if((filter->pred.lhs->operand.type == AR_EXP_CONSTANT ||
-				filter->pred.rhs->operand.type == AR_EXP_CONSTANT) &&
-			   (filter->pred.lhs->operand.type == AR_EXP_VARIADIC ||
-				filter->pred.lhs->operand.type == AR_EXP_VARIADIC)) {
-
+			SIValue v_lhs = SI_NullVal();
+			SIValue v_rhs = SI_NullVal();
+			bool lhs_scalar = AR_EXP_ReduceToScalar(filter->pred.lhs, true, &v_lhs);
+			bool rhs_scalar = AR_EXP_ReduceToScalar(filter->pred.rhs, true, &v_rhs);
+			// Predicate should be in the form of variable=scalar or scalar=variadic
+			if((lhs_scalar && !rhs_scalar) || (!lhs_scalar && rhs_scalar)) {
 				// Validate constant type.
-				SIValue c = SI_NullVal();
-				if(filter->pred.lhs->operand.type == AR_EXP_CONSTANT) c = filter->pred.lhs->operand.constant;
-				if(filter->pred.rhs->operand.type == AR_EXP_CONSTANT) c = filter->pred.rhs->operand.constant;
+				SIValue c = lhs_scalar ? v_lhs : v_rhs;
 				SIType t = SI_TYPE(c);
-
 				res = (t & (SI_NUMERIC | T_STRING | T_BOOL));
 			}
 		}

tests/flow/test_params.py

@@ -97,3 +97,14 @@ def test_missing_parameter(self):
             # Expecting an error.
             pass
 
+    def test_id_scan(self):
+        redis_graph.query("CREATE ({val:1})")
+        expected_results=[[1]]
+        params = {'id' : 0}
+        query = "MATCH (n) WHERE id(n)=$id return n.val"
+        query_info = QueryInfo(query = query, description="Test id scan with params", expected_result = expected_results)
+        self._assert_resultset_equals_expected(redis_graph.query(query, params), query_info)
+        query = redis_graph.build_params_header(params) + query
+        plan = redis_graph.execution_plan(query)
+        self.env.assertIn('NodeByIdSeek', plan)
+