From: imeevma@tarantool.org To: alexander.turenko@tarantool.org Cc: tarantool-patches@dev.tarantool.org Subject: [Tarantool-patches] [PATCH v2 test-run 1/1] sql: add another way to change SQL default engine Date: Sun, 29 Dec 2019 17:20:18 +0300 [thread overview] Message-ID: <c0f30b2e1e5fe6155c9812d1c7188941b6d2eab5.1577629010.git.imeevma@gmail.com> (raw) Thank you for review! My answers and new patch below. On 12/28/19 1:57 PM, Alexander Turenko wrote: > I don't mind in general, but please consider several notes below. > > WBR, Alexander Turenko. > > On Fri, Dec 27, 2019 at 03:33:12PM +0300, imeevma@tarantool.org wrote: >> sql: add another way to change SQL default engine >> >> Due to the removal of the sql_default_engine pragma in 2.3.1, this >> patch adds another way to change this setting. > > Nit: I would say 'implements' rather then 'adds', because the letter can > be read as something configurable by a user. > Fixed. > I would mention that it affects only *.test.sql tests, just in case. > Done. > I think it would be good to link the issue about it here. > Done. >> --- >> https://github.com/tarantool/test-run/tree/imeevma/fix_sql_default_engine >> >> lib/tarantool_server.py | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py >> index 6f71b5b..20a398e 100644 >> --- a/lib/tarantool_server.py >> +++ b/lib/tarantool_server.py >> @@ -145,12 +145,19 @@ class LuaTest(Test): >> return True >> >> engine = self.run_params['engine'] >> - command = "pragma sql_default_engine='{}'".format(engine) >> - result = self.send_command(command, ts, 'sql') >> - result = result.replace('\r\n', '\n') >> - if result != '---\n- row_count: 0\n...\n': >> - sys.stdout.write(result) >> - return False >> + >> + command_new = ("UPDATE \"_session_settings\" SET \"value\" = '{}' " + \ >> + "WHERE \"name\" = 'sql_default_engine'").format(engine) >> + result_new = self.send_command(command_new, ts, 'sql') >> + result_new = result_new.replace('\r\n', '\n') >> + >> + command_old = "pragma sql_default_engine='{}'".format(engine) >> + result_old = self.send_command(command_old, ts, 'sql') >> + result_old = result_old.replace('\r\n', '\n') >> + if result_new != '---\n- row_count: 1\n...\n': >> + if result_old != '---\n- row_count: 0\n...\n': >> + sys.stdout.write(result_old) >> + return False > > I would make it like so: > > 1. Send "pragma ...", await a result. > - If succeeds, return True. > - Otherwise proceed with the step 2. > 2. Send "UPDATE ...", await a result. > - If succeeds, return True. > - Otherwise return False. > Fixed. > This way we'll save one extra hop on a fresh tarantool. Not so big > overhead for an action that is run once for a test, but we can overcome > it and pay nothing, so it looks worthful for me. > > Please, look into CI: it reports a flake8 error. You can use `make lint` > locally to verify python code style. > Fixed. > Please, double-check that everything works as expected for both > tarantool versions (with pragma support and with _session_settings): > let's run a .test.sql test and verify that it creates spaces in an > expected engine. It should be easier using `\set language lua` command: > it is simple to assess a space engine and compare it with > test_run:get_cfg('engine'). > Checked. I haven't found any errors. > README states that test-run use "pragma <...>" to set an engine. It > would be good to mention the new way together with the old one. > Fixed. >> >> return True >> >> -- >> 2.7.4 > > BTW, the subject of the email is the following: > > | [PATCH 1/1] sql: add another way to change SQL default engine > > I suggest to add a repo prefix when it is not tarantool itself. Look as > Vlad does it: > > | [PATCH vshard 1/1] travis-ci: deploy packages from tagged revisions > > https://lists.tarantool.org/pipermail/tarantool-patches/2019-August/001304.html Fixed. New patch: From c0f30b2e1e5fe6155c9812d1c7188941b6d2eab5 Mon Sep 17 00:00:00 2001 From: Mergen Imeev <imeevma@gmail.com> Date: Sun, 29 Dec 2019 16:48:29 +0300 Subject: [PATCH] sql: add another way to change SQL default engine Currently, the pragma sql_default_engine is used in the *.test.sql tests to change the default SQL engine. But in the issue tarantool/tarantool#4511, this pragma will be removed. To avoid failures in these tests because of this, in addition to the current one, another way to change this parameter was implemented. diff --git a/README.md b/README.md index aefeeff..781d003 100644 --- a/README.md +++ b/README.md @@ -69,9 +69,10 @@ engine = test_run:get_cfg('engine') ``` "engine" value has a special meaning for *.test.sql files: if it is "memtx" or -"vinyl", then the corresponding engine will be set using "pragma -sql_default_engine='memtx|vinyl'" command before executing commands from a test -file. +"vinyl", then the corresponding engine will be set using either "pragma +sql_default_engine='memtx|vinyl'" or "UPDATE \\"_session_settings\\" SET +\\"value\\" = 'memtx|vinyl' WHERE \\"name\\" = 'sql_default_engine';" command +before executing commands from a test file. #### Python diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py index 6f71b5b..0b83c2e 100644 --- a/lib/tarantool_server.py +++ b/lib/tarantool_server.py @@ -148,7 +148,15 @@ class LuaTest(Test): command = "pragma sql_default_engine='{}'".format(engine) result = self.send_command(command, ts, 'sql') result = result.replace('\r\n', '\n') - if result != '---\n- row_count: 0\n...\n': + if result != '---\n- row_count: 0\n...\n' and result != "---\n- " +\ + "null\n- Pragma 'SQL_DEFAULT_ENGINE' does not exist\n...\n": + sys.stdout.write(result) + return False + command = ("UPDATE \"_session_settings\" SET \"value\" = '{}' " + + "WHERE \"name\" = 'sql_default_engine';").format(engine) + result = self.send_command(command, ts, 'sql') + result = result.replace('\r\n', '\n') + if result != '---\n- row_count: 1\n...\n': sys.stdout.write(result) return False
next reply other threads:[~2019-12-29 14:20 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-29 14:20 imeevma [this message] 2019-12-30 3:09 ` Alexander Turenko 2019-12-30 9:55 ` Mergen Imeev 2019-12-30 17:14 ` Alexander Turenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c0f30b2e1e5fe6155c9812d1c7188941b6d2eab5.1577629010.git.imeevma@gmail.com \ --to=imeevma@tarantool.org \ --cc=alexander.turenko@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 test-run 1/1] sql: add another way to change SQL default engine' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox