Skip to content

Commit

Permalink
ENG-2655: Fix issue when binding IN args list for full hash key
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@robertpang
robertpang committed Dec 23, 2017
1 parent 49e56d6 commit 0365a89
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 26 deletions.
58 changes: 54 additions & 4 deletions java/yb-cql/src/test/java/org/yb/cql/TestBindVariable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1760,6 +1760,56 @@ public void testInKeywordWithBind() throws Exception {
assertTrue(expectedValues.isEmpty());
}

// Test binding full hash key.
{
List<Integer> intList = new LinkedList<>();
intList.add(1);
intList.add(-1);
intList.add(3);
intList.add(7);

List<String> 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<Integer> 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<String, Object>() {{
put("b1", intList);
put("b2", strList);
}});
Set<Integer> 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<String> stringList = new LinkedList<>();
Expand All @@ -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<Integer> expectedValues = new HashSet<>();
expectedValues.add(1);
Expand All @@ -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<Integer> expectedValues = new HashSet<>();
expectedValues.add(2);
// Check rows
Expand All @@ -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<Integer> expectedValues = new HashSet<>();
expectedValues.add(5);
// Check rows
Expand All @@ -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<Integer> expectedValues = new HashSet<>();
expectedValues.add(4);
expectedValues.add(9);
Expand Down
45 changes: 23 additions & 22 deletions src/yb/yql/cql/ql/exec/eval_where.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,32 +152,33 @@ CHECKED_STATUS Executor::WhereClauseToPB(QLReadRequestPB *req,
is_multi_partition = true;
partitions_count = 1;
}
auto *in_expr = static_cast<const PTCollectionExpr *>(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<QLValuePB> set_values;
for (const QLValuePB &value_pb : col_pb.value().list_value().elems()) {
set_values.insert(value_pb);
}
std::set<QLValuePB> 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;
}
Expand Down

0 comments on commit 0365a89

Please sign in to comment.