From 0365a892f4c63a6b19362d705a90379a38288685 Mon Sep 17 00:00:00 2001 From: Robert Pang Date: Fri, 22 Dec 2017 14:23:24 -0800 Subject: [PATCH] ENG-2655: Fix issue when binding IN args list for full hash key Summary: We have a fast-path to return no results when the argument list for the 'IN' is empty. We now check for emptiness only after evaluating the expression to ensure that the bound variables are resolved. Previously, we were checking before and, therefore, wrongly returned no results in this case. Test Plan: jenkins, updated TestBindVariable Reviewers: kannan, robert Reviewed By: kannan Subscribers: kannan, yql Differential Revision: https://phabricator.dev.yugabyte.com/D3744 --- .../java/org/yb/cql/TestBindVariable.java | 58 +++++++++++++++++-- src/yb/yql/cql/ql/exec/eval_where.cc | 45 +++++++------- 2 files changed, 77 insertions(+), 26 deletions(-) diff --git a/java/yb-cql/src/test/java/org/yb/cql/TestBindVariable.java b/java/yb-cql/src/test/java/org/yb/cql/TestBindVariable.java index 722f13963deb..4e3a08368201 100644 --- a/java/yb-cql/src/test/java/org/yb/cql/TestBindVariable.java +++ b/java/yb-cql/src/test/java/org/yb/cql/TestBindVariable.java @@ -1760,6 +1760,56 @@ public void testInKeywordWithBind() throws Exception { assertTrue(expectedValues.isEmpty()); } + // Test binding full hash key. + { + List intList = new LinkedList<>(); + intList.add(1); + intList.add(-1); + intList.add(3); + intList.add(7); + + List strList = new LinkedList<>(); + strList.add("h1"); + strList.add("h3"); + strList.add("h7"); + strList.add("h10"); + + { + ResultSet rs = session.execute("SELECT * FROM in_bind_test WHERE h1 IN ? AND h2 IN ?", + intList, strList); + Set expectedValues = new HashSet<>(); + expectedValues.add(1); + expectedValues.add(3); + expectedValues.add(7); + // Check rows + for (Row row : rs) { + Integer h1 = row.getInt("h1"); + assertTrue(expectedValues.contains(h1)); + expectedValues.remove(h1); + } + assertTrue(expectedValues.isEmpty()); + } + + { + ResultSet rs = session.execute("SELECT * FROM in_bind_test WHERE h1 IN :b1 AND h2 IN :b2", + new HashMap() {{ + put("b1", intList); + put("b2", strList); + }}); + Set expectedValues = new HashSet<>(); + expectedValues.add(1); + expectedValues.add(3); + expectedValues.add(7); + // Check rows + for (Row row : rs) { + Integer h1 = row.getInt("h1"); + assertTrue(expectedValues.contains(h1)); + expectedValues.remove(h1); + } + assertTrue(expectedValues.isEmpty()); + } + } + // Test prepare bind. { List stringList = new LinkedList<>(); @@ -1769,7 +1819,7 @@ public void testInKeywordWithBind() throws Exception { stringList.add("r101"); PreparedStatement prepared = - session.prepare("SELECT * FROM in_bind_test WHERE r2 IN ?"); + session.prepare("SELECT * FROM in_bind_test WHERE r2 IN ?"); ResultSet rs = session.execute(prepared.bind(stringList)); Set expectedValues = new HashSet<>(); expectedValues.add(1); @@ -1787,7 +1837,7 @@ public void testInKeywordWithBind() throws Exception { // Test binding IN elems individually - one element { ResultSet rs = session.execute("SELECT * FROM in_bind_test WHERE h1 IN (?)", - new Integer(2)); + new Integer(2)); Set expectedValues = new HashSet<>(); expectedValues.add(2); // Check rows @@ -1802,7 +1852,7 @@ public void testInKeywordWithBind() throws Exception { // Test binding IN elems individually - multiple conditions { ResultSet rs = session.execute("SELECT * FROM in_bind_test WHERE h1 IN (?) AND r1 IN (?)", - new Integer(5), new Integer(105)); + new Integer(5), new Integer(105)); Set expectedValues = new HashSet<>(); expectedValues.add(5); // Check rows @@ -1817,7 +1867,7 @@ public void testInKeywordWithBind() throws Exception { // Test binding IN elems individually - several elements { ResultSet rs = session.execute("SELECT * FROM in_bind_test WHERE h1 IN (?, ?, ?)", - new Integer(9), new Integer(4), new Integer(-1)); + new Integer(9), new Integer(4), new Integer(-1)); Set expectedValues = new HashSet<>(); expectedValues.add(4); expectedValues.add(9); diff --git a/src/yb/yql/cql/ql/exec/eval_where.cc b/src/yb/yql/cql/ql/exec/eval_where.cc index 2ada8b75ea35..8cf0e69d3e97 100644 --- a/src/yb/yql/cql/ql/exec/eval_where.cc +++ b/src/yb/yql/cql/ql/exec/eval_where.cc @@ -152,32 +152,33 @@ CHECKED_STATUS Executor::WhereClauseToPB(QLReadRequestPB *req, is_multi_partition = true; partitions_count = 1; } - auto *in_expr = static_cast(op.expr().get()); - if (in_expr->size() == 0) { + + // De-duplicating and ordering values from the 'IN' expression. + QLExpressionPB col_pb; + Status s = PTExprToPB(op.expr(), &col_pb); + if (PREDICT_FALSE(!s.ok())) { + return exec_context_->Error(s, ErrorCode::INVALID_ARGUMENTS); + } + + // Fast path for returning no results when 'IN' list is empty. + if (col_pb.value().list_value().elems_size() == 0) { *no_results = true; return Status::OK(); - } else { - // De-duplicating and ordering values from the 'IN' expression. - QLExpressionPB col_pb; - Status s = PTExprToPB(op.expr(), &col_pb); - if (PREDICT_FALSE(!s.ok())) { - return exec_context_->Error(s, ErrorCode::INVALID_ARGUMENTS); - } + } - std::set set_values; - for (const QLValuePB &value_pb : col_pb.value().list_value().elems()) { - set_values.insert(value_pb); - } + std::set set_values; + for (const QLValuePB &value_pb : col_pb.value().list_value().elems()) { + set_values.insert(value_pb); + } - // Adding partition options information to the execution context. - partitions_count *= set_values.size(); - exec_context_->hash_values_options()->emplace_back(); - auto& options = exec_context_->hash_values_options()->back(); - for (const QLValuePB &value_pb : set_values) { - options.emplace_back(); - options.back().set_column_id(col_desc->id()); - options.back().mutable_value()->CopyFrom(value_pb); - } + // Adding partition options information to the execution context. + partitions_count *= set_values.size(); + exec_context_->hash_values_options()->emplace_back(); + auto& options = exec_context_->hash_values_options()->back(); + for (const QLValuePB &value_pb : set_values) { + options.emplace_back(); + options.back().set_column_id(col_desc->id()); + options.back().mutable_value()->CopyFrom(value_pb); } break; }