[Maria-developers] MDEV-17399: JSON_TABLE: final input
Hi Alexey, Please find below final bits of input. After these are addressed the patch will probably be good to push (But I'll need a final pass before giving ok to push). == json_table_mysql* tests == * Please remove one of the json_table_mysql and json_table_mysql2 tests. * Please move the remaining test to be in the same suite as json_table.test. == Fix json_table.test == The test has this somewhere in the middle: set optimizer_switch='firstmatch=off'; Please restore the original setting. == ha_json_table::rnd_next() returns 0 on error == Consider this query: select * from json_table( '[ {"name": "X"}, {"name2": "Y"} ]', '$[*]' columns ( col1 varchar(100) path '$.name2' error on empty )) as T; The query produces an error. It happens like so: * the call to ha_json_table::rnd_next() returns 0. * The SQL layer finds out about the error by checking thd->is_error() which returns true. Please make ha_json_table::rnd_next() return an error. == Rename table_function.* == Looking at the contets of sql/table_function.{h,cc}, one can see that there's very little code there that is applicable to generic table function. Almost all code is specifi to JSON_TABLE. Because of that, please rename the files to e.g. json_table.{h,cc}. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
Hi, Sergei! I pushed the patch to the feature branch for you to take a look. The patch you proposed http://lists.askmonty.org/pipermail/commits/2021-March/014492.html I liked and adapted with one exception. The nested paths list is built using the **last_sibling_hook instead of *cur_last_sibling. That seems to me nicer and doesn't produce that many repeating lines in the code. Best regards. HF On Thu, Mar 4, 2021 at 3:11 PM Sergey Petrunia <sergey@mariadb.com> wrote:
Hi Alexey,
Please find below final bits of input. After these are addressed the patch will probably be good to push (But I'll need a final pass before giving ok to push).
== json_table_mysql* tests ==
* Please remove one of the json_table_mysql and json_table_mysql2 tests. * Please move the remaining test to be in the same suite as json_table.test.
== Fix json_table.test ==
The test has this somewhere in the middle:
set optimizer_switch='firstmatch=off';
Please restore the original setting.
== ha_json_table::rnd_next() returns 0 on error ==
Consider this query:
select * from json_table( '[ {"name": "X"}, {"name2": "Y"} ]', '$[*]' columns ( col1 varchar(100) path '$.name2' error on empty )) as T;
The query produces an error. It happens like so:
* the call to ha_json_table::rnd_next() returns 0. * The SQL layer finds out about the error by checking thd->is_error() which returns true.
Please make ha_json_table::rnd_next() return an error.
== Rename table_function.* ==
Looking at the contets of sql/table_function.{h,cc}, one can see that there's very little code there that is applicable to generic table function. Almost all code is specifi to JSON_TABLE. Because of that, please rename the files to e.g. json_table.{h,cc}.
BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
Hi Alexey, On Sat, Mar 06, 2021 at 01:13:53AM +0400, Alexey Botchkov wrote:
Hi, Sergei!
I pushed the patch to the feature branch for you to take a look.
This broke CI, please fix: http://buildbot.askmonty.org/buildbot/builders/kvm-rpm-centos74-amd64/builds... CMake Error at cmake/libutils.cmake:108 (ADD_LIBRARY): Cannot find source file: ../sql/table_function.cc BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
forgot about the embedded server. Fixed. On Wed, Mar 10, 2021 at 12:47 PM Sergey Petrunia <sergey@mariadb.com> wrote:
Hi Alexey,
On Sat, Mar 06, 2021 at 01:13:53AM +0400, Alexey Botchkov wrote:
Hi, Sergei!
I pushed the patch to the feature branch for you to take a look.
This broke CI, please fix:
http://buildbot.askmonty.org/buildbot/builders/kvm-rpm-centos74-amd64/builds...
CMake Error at cmake/libutils.cmake:108 (ADD_LIBRARY): Cannot find source file:
../sql/table_function.cc BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
On Sat, Mar 06, 2021 at 01:13:53AM +0400, Alexey Botchkov wrote:
Hi, Sergei!
I pushed the patch to the feature branch for you to take a look. The patch you proposed http://lists.askmonty.org/pipermail/commits/2021-March/014492.html I liked and adapted with one exception. The nested paths list is built using the **last_sibling_hook instead of *cur_last_sibling. That seems to me nicer and doesn't produce that many repeating lines in the code.
On the other hand, use of "last_sibling_hook" makes Table_function_json_table to be aware about the internals of Json_table_nested_path (you had to make it a friend class). And the price to pay for complete isolation was the if-else in start_nested_path(), with two lines in either branch. So I would still say that the suggested solution was cleaner. I don't consider this to be a showstopper issue, though. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
The Json_table_nested_path lives only inside the Table_function_json_table and that last one is the only class that manipulates the Json_table_nested_path internals. Using methods in your version, but it's the only class using these methods anyway. So to me it's seems more honest to declare classes as friends so we explicitly allow access to the internals to that one class and disallow to everyone else. But yes, it's rather the matter of taste. Best regards. HF On Fri, Mar 12, 2021 at 2:33 AM Sergey Petrunia <sergey@mariadb.com> wrote:
On Sat, Mar 06, 2021 at 01:13:53AM +0400, Alexey Botchkov wrote:
Hi, Sergei!
I pushed the patch to the feature branch for you to take a look. The patch you proposed http://lists.askmonty.org/pipermail/commits/2021-March/014492.html I liked and adapted with one exception. The nested paths list is built using the **last_sibling_hook instead of *cur_last_sibling. That seems to me nicer and doesn't produce that many repeating lines in the code.
On the other hand, use of "last_sibling_hook" makes Table_function_json_table to be aware about the internals of Json_table_nested_path (you had to make it a friend class).
And the price to pay for complete isolation was the if-else in start_nested_path(), with two lines in either branch. So I would still say that the suggested solution was cleaner.
I don't consider this to be a showstopper issue, though.
BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
participants (2)
-
Alexey Botchkov -
Sergey Petrunia