Skip to content

Commit

Permalink
Bug#26496210: UNNECESSARY LOOPS THROUGH VECTORS OF SIZE 1
Browse files Browse the repository at this point in the history 
JSON_ARRAY_APPEND, JSON_INSERT and JSON_ARRAY_INSERT reject path
arguments that may match more than one value. Still, they loop over
the vector of search results, whose size is known to be 1. For
historical reasons, they even iterate over it backwards.

This patch simplifies the code by removing the backwards iteration
over the vector. Instead, the first element of the vector is accessed
directly.

Change-Id: Idc9d3b7c594f0ad3bd8424af5fbeb76eee35ca8e
  • Loading branch information
@kahatlen
kahatlen committed Aug 7, 2017
1 parent 07fdc81 commit 53a4a44
Showing 1 changed file with 105 additions and 142 deletions.
247 changes: 105 additions & 142 deletions sql/item_json_func.cc
View file
Expand Up @@ -2120,62 +2120,50 @@ bool Item_func_json_array_append::val_json(Json_wrapper *wr)
if (doc->seek(*path, &hits, true, true))
return error_json(); /* purecov: inspected */

if (hits.size() < 1)
{
if (hits.empty())
continue;
}

/*
Iterate backwards lest we get into trouble with replacing outer
parts of the doc before we get to paths to inner parts when we have
ellipses in the path. Make sure we do the most nested replacements
first. Json_dom::seek returns outermost hits first.
// Paths with wildcards and ranges are rejected, so expect one hit.
DBUG_ASSERT(hits.size() == 1);
Json_dom *hit= hits[0];

Note that, later on, we decide to forbid ellipses in the path
arguments to json_array_append().
*/
for (Json_dom_vector::iterator it= hits.end(); it != hits.begin();)
{
--it;
Json_wrapper valuew;
if (get_atom_null_as_null(args, i + 1, func_name(), &m_value,
&m_conversion_buffer,
&valuew))
return error_json();
Json_wrapper valuew;
if (get_atom_null_as_null(args, i + 1, func_name(), &m_value,
&m_conversion_buffer, &valuew))
return error_json();

Json_dom_ptr val_dom(valuew.to_dom(thd));
valuew.set_alias(); // we have taken over the DOM
Json_dom_ptr val_dom(valuew.to_dom(thd));
valuew.set_alias(); // we have taken over the DOM

if ((*it)->json_type() == enum_json_type::J_ARRAY)
if (hit->json_type() == enum_json_type::J_ARRAY)
{
Json_array *arr= down_cast<Json_array *>(hit);
if (arr->append_alias(std::move(val_dom)))
return error_json(); /* purecov: inspected */
}
else
{
Json_array_ptr arr(new (std::nothrow) Json_array());
if (arr == nullptr ||
arr->append_clone(hit) ||
arr->append_alias(std::move(val_dom)))
{
Json_array *arr= down_cast<Json_array *>(*it);
if (arr->append_alias(std::move(val_dom)))
return error_json(); /* purecov: inspected */
return error_json(); /* purecov: inspected */
}
/*
This value will replace the old document we found using path, since
we did an auto-wrap. If this is root, this is trivial, but if it's
inside an array or object, we need to find the parent DOM to be
able to replace it in situ.
*/
if (wrapped_top_level_item(path, hit))
{
docw= Json_wrapper(std::move(arr));
}
else
{
Json_array_ptr arr(new (std::nothrow) Json_array());
if (arr == nullptr ||
arr->append_clone(*it) ||
arr->append_alias(std::move(val_dom)))
{
return error_json(); /* purecov: inspected */
}
/*
This value will replace the old document we found using path, since
we did an auto-wrap. If this is root, this is trivial, but if it's
inside an array or object, we need to find the parent DOM to be
able to replace it in situ.
*/
if (wrapped_top_level_item(path, (*it)))
{
docw= Json_wrapper(std::move(arr));
}
else
{
Json_dom *parent= (*it)->parent();
parent->replace_dom_in_container(*it, std::move(arr));
}
Json_dom *parent= hit->parent();
parent->replace_dom_in_container(hit, std::move(arr));
}
}
}
Expand Down Expand Up @@ -2266,84 +2254,72 @@ bool Item_func_json_insert::val_json(Json_wrapper *wr)
continue;
}

// We found *something* at that parent path

Json_wrapper valuew;
if (get_atom_null_as_null(args, i + 1, func_name(), &m_value,
&m_conversion_buffer,
&valuew))
&m_conversion_buffer, &valuew))
{
return error_json();
}

/*
Iterate backwards lest we get into trouble with replacing outer
parts of the doc before we get to paths to inner parts when we have
ellipses in the path. Make sure we do the most nested replacements
first. Json_dom::seek returns outermost hits first.
// Paths with wildcards and ranges are rejected, so expect one hit.
DBUG_ASSERT(hits.size() == 1);
Json_dom *hit= hits[0];

Note that, later on, we decided to forbid ellipses in the path
arguments to json_insert().
*/
for (Json_dom_vector::iterator it= hits.end(); it != hits.begin();)
// What did we specify in the path, object or array?
if (leg->get_type() == jpl_array_cell)
{
--it;
// We found *something* at that parent path

// What did we specify in the path, object or array?
if (leg->get_type() == jpl_array_cell)
// We specified an array, what did we find at that position?
if (hit->json_type() == enum_json_type::J_ARRAY)
{
// We found an array, so either prepend or append.
Json_array *arr= down_cast<Json_array *>(hit);
size_t pos= leg->first_array_index(arr->size()).position();
if (arr->insert_alias(pos, valuew.clone_dom(thd)))
return error_json(); /* purecov: inspected */
}
else if (!leg->is_autowrap())
{
// We specified an array, what did we find at that position?
if ((*it)->json_type() == enum_json_type::J_ARRAY)
/*
Found a scalar or object and we didn't specify position 0 or last:
auto-wrap it and either prepend or append.
*/
size_t pos= leg->first_array_index(1).position();
Json_array_ptr newarr(new (std::nothrow) Json_array());
if (newarr == nullptr ||
newarr->append_clone(hit) /* auto-wrap this */ ||
newarr->insert_alias(pos, valuew.clone_dom(thd)))
{
// We found an array, so either prepend or append.
Json_array *arr= down_cast<Json_array *>(*it);
size_t pos= leg->first_array_index(arr->size()).position();
if (arr->insert_alias(pos, valuew.clone_dom(thd)))
return error_json(); /* purecov: inspected */
return error_json(); /* purecov: inspected */
}
else if (!leg->is_autowrap())
{
/*
Found a scalar or object and we didn't specify position 0 or last:
auto-wrap it and either prepend or append.
*/
size_t pos= leg->first_array_index(1).position();
Json_dom *a= *it;
Json_array_ptr newarr(new (std::nothrow) Json_array());
if (newarr == nullptr ||
newarr->append_clone(a) /* auto-wrap this */ ||
newarr->insert_alias(pos, valuew.clone_dom(thd)))
{
return error_json(); /* purecov: inspected */
}

/*
Now we need this value to replace the old document we found using
path. If this is root, this is trivial, but if it's inside an
array or object, we need to find the parent DOM to be able to
replace it in situ.
*/
if (m_path.leg_count() == 0) // root
{
docw= Json_wrapper(std::move(newarr));
}
else
{
Json_dom *parent= a->parent();
DBUG_ASSERT(parent);
/*
Now we need this value to replace the old document we found using
path. If this is root, this is trivial, but if it's inside an
array or object, we need to find the parent DOM to be able to
replace it in situ.
*/
if (m_path.leg_count() == 0) // root
{
docw= Json_wrapper(std::move(newarr));
}
else
{
Json_dom *parent= hit->parent();
DBUG_ASSERT(parent);

parent->replace_dom_in_container(a, std::move(newarr));
}
parent->replace_dom_in_container(hit, std::move(newarr));
}
}
else if (leg->get_type() == jpl_member &&
(*it)->json_type() == enum_json_type::J_OBJECT)
{
Json_object *o= down_cast<Json_object *>(*it);
if (o->add_clone(leg->get_member_name(), valuew.to_dom(thd)))
return error_json(); /* purecov: inspected */
}
}

else if (leg->get_type() == jpl_member &&
hit->json_type() == enum_json_type::J_OBJECT)
{
Json_object *o= down_cast<Json_object *>(hit);
if (o->add_clone(leg->get_member_name(), valuew.to_dom(thd)))
return error_json(); /* purecov: inspected */
}
} // end of loop through paths
// docw still owns the augmented doc, so hand it over to result
*wr= std::move(docw);
Expand Down Expand Up @@ -2425,40 +2401,29 @@ bool Item_func_json_array_insert::val_json(Json_wrapper *wr)
continue;
}

// We found *something* at that parent path

// Paths with wildcards and ranges are rejected, so expect one hit.
DBUG_ASSERT(hits.size() == 1);
Json_dom *hit= hits[0];

// NOP if parent is not an array
if (hit->json_type() != enum_json_type::J_ARRAY)
continue;

Json_wrapper valuew;
if (get_atom_null_as_null(args, i + 1, func_name(),
&m_value, &m_conversion_buffer,
&valuew))
if (get_atom_null_as_null(args, i + 1, func_name(), &m_value,
&m_conversion_buffer, &valuew))
{
return error_json();
}

/*
Iterate backwards lest we get into trouble with replacing outer
parts of the doc before we get to paths to inner parts when we have
ellipses in the path. Make sure we do the most nested replacements
first. Json_dom::seek returns outermost hits first.
Note that, later on, we decided to forbid ellipses in the path
arguments to json_insert().
*/
for (Json_dom_vector::iterator it= hits.end(); it != hits.begin();)
{
--it;
// We found *something* at that parent path

// NOP if parent is not an array

if ((*it)->json_type() == enum_json_type::J_ARRAY)
{
// Excellent. Insert the value at that location.
Json_array *arr= down_cast<Json_array *>(*it);
DBUG_ASSERT(leg->get_type() == jpl_array_cell);
size_t pos= leg->first_array_index(arr->size()).position();
if (arr->insert_alias(pos, valuew.clone_dom(thd)))
return error_json(); /* purecov: inspected */
}
}
// Insert the value at that location.
Json_array *arr= down_cast<Json_array *>(hit);
DBUG_ASSERT(leg->get_type() == jpl_array_cell);
size_t pos= leg->first_array_index(arr->size()).position();
if (arr->insert_alias(pos, valuew.clone_dom(thd)))
return error_json(); /* purecov: inspected */

} // end of loop through paths
// docw still owns the augmented doc, so hand it over to result
Expand Down Expand Up @@ -2646,8 +2611,7 @@ bool Item_func_json_set_replace::val_json(Json_wrapper *wr)

Json_wrapper valuew;
if (get_atom_null_as_null(args, i + 1, func_name(), &m_value,
&m_conversion_buffer,
&valuew))
&m_conversion_buffer, &valuew))
return error_json();

if (binary_diffs)
Expand Down Expand Up @@ -2881,8 +2845,7 @@ bool Item_func_json_array::val_json(Json_wrapper *wr)
{
Json_wrapper valuew;
if (get_atom_null_as_null(args, i, func_name(), &m_value,
&m_conversion_buffer,
&valuew))
&m_conversion_buffer, &valuew))
{
return error_json();
}
Expand Down

0 comments on commit 53a4a44

Please sign in to comment.