Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL statement terminator should be semicolon. #2125

Closed
kyukhin opened this issue Mar 3, 2017 · 6 comments
Closed

SQL statement terminator should be semicolon. #2125

kyukhin opened this issue Mar 3, 2017 · 6 comments
Assignees
Labels
good first issue Good for newcomers
Milestone

Comments

@kyukhin
Copy link
Contributor

kyukhin commented Mar 3, 2017

In order to have multi-line SQL statements, and avoid confusion let's make ;mandatory statement terminator.

I am able to say
select * from ww
there is no semicolon. This might look
convenient, but what is somebody implements
a dumb client?
Currently, If I have a multi-line statement,
there's no error
"
tarantool> ocelot_conn:execute('select * from
> ww')
--
...
"
But that won't last -- people will want to
type in SQL, e.g.
tarantool-sql> select * from
> ww
If ";" isn't the end-of-statement delimiter,
and newline is a signal for execution, then
the dumb client will try to execute
"select * from" and fail.
Of course, it's always hard to know when an
SQL statement is really supposed to end -- only
a sophisticated client can detect that.
But probably it would help if statements
had to end with semicolons.

@kostja kostja modified the milestone: 1.8.1 Mar 29, 2017
@kostja kostja modified the milestones: 1.8.3, 1.8.2 May 31, 2017
@kostja kostja added the good first issue Good for newcomers label Feb 7, 2018
@kshcherbatov
Copy link
Contributor

This functionality seems to be already in Tarantool:
There is "pragma delimiter" introduced woth @GeorgyKirichenko patch c3afd0 "Add support for backslash commands in console".
Usage scenario:

tarantool> \set delimiter ;
---
...

tarantool> \set language sql
---
- true
...

tarantool> CREATE TABLE t2  (a int primary key,
         > b int)
         > ;

@Gerold103
Copy link
Collaborator

Gerold103 commented Jun 19, 2018

tarantool> \set delimiter ;
---
...

This must be default behaviour. A user has not to set delimiter ; manually.
And please, test, what will happen, for example, on a request like this:

select 'select "abcd";';

@Gerold103
Copy link
Collaborator

I have found two problems:

  1. When I use arrows on my keyboard to get a previous statement, its delimiter ; is truncated:
tarantool> select 'select "abcd";';
---
- - ['select "abcd";']
...

tarantool> select 'select "abcd";'

Here I got select 'select "abcd";' by pressing keyboard button 'arrow up'. I think, that such history is unusable. It must contain terminating ; as well.

  1. Spaces after ; surpass it.
tarantool> select 'select "abcd";';     
         > 

Here I put several spaces after ; and disabled its termination meaning.

@kshcherbatov
Copy link
Contributor

kshcherbatov commented Jun 19, 2018

Thank you for the comments,
as for first comment I've accounted it in the patch bellow.

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index e7cb50a..7644d33 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -67,6 +67,7 @@ local function set_language(storage, value)
         local msg = 'Invalid language "%s", supported languages: lua and sql.'
         return error(msg:format(value))
     end
+    set_delimiter(storage, value == 'sql' and ';' or nil)
     storage.language = value
     return true
 end
@@ -316,7 +317,11 @@ local function local_read(self)
                 break
             end
         elseif #buf >= #delim and buf:sub(#buf - #delim + 1) == delim then
-            buf = buf:sub(0, #buf - #delim)
+            local sub_len = #buf - #delim
+            if box.session.language == 'sql' and delim == ';' then
+                sub_len = sub_len + 1
+            end
+            buf = buf:sub(0, sub_len)
             break
         end
         buf = buf.."\n"
@@ -354,8 +359,12 @@ local function client_read(self)
         -- Escape sequence to close current connection (like SSH)
         return nil
     end
-    -- remove trailing delimiter
-    return buf:sub(1, -#delim-1)
+    -- remove trailing delimiter if it is not SQL ;
+    if box.session.language == 'sql' and delim == ';' then
+        return buf
+    else
+        return buf:sub(1, -#delim-1)
+    end
 end

As for second, we should trim every new data portion on read from console or socket using smth like this:

function rtrim(s)
  local n = #s
  while n > 0 and s:find("^%s", n) do n = n - 1 end
  return s:sub(1, n)
end

But I am not shure that it is a good idea.

@Gerold103
Copy link
Collaborator

Please, use the mailing list to send patches. I can not review them here.

kshcherbatov added a commit that referenced this issue Jun 19, 2018
Now semicolon sets as a default terminator on entering
"set language sql".

Resolves #2125.
@kshcherbatov
Copy link
Contributor

won't implement for now
@kostja confirmed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants