-
-
Notifications
You must be signed in to change notification settings - Fork 243
Feature SET AUTOTERM in ISQL #7868
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
Conversation
recreate package body PACKAGE1
as
begin
procedure P001
(a1 integer not null=0,
a2 double precision=3,
a3 character varying(12) character set utf8 not null collate utf8 = _win1251 x'ABCDEF')
returns
(r1 integer not null,
r2 double precision,
r3 character varying(12) character set utf8 not null collate utf8);
procedure P001
(a1 integer not null=0,
a2 double precision=3,
a3 character varying(12) character set utf8 not null collate utf8 = _win1251 x'ABCDEF')
returns
(r1 integer not null,
r2 double precision,
r3 character varying(12) character set utf8 not null collate utf8)
as
declare procedure SUB_SP1(aa integer) returns (bb double precision)
as
declare procedure SUB2_SP1(aa integer) returns (bb double precision)
as
declare procedure SUB3_SP1(aa integer) returns (bb double precision)
as
declare procedure SUB4_SP1(aa integer) returns (bb double precision)
as
begin
bb=case when (aa)<(case when (aa)<1 then (1) else (case when (aa)<1 then (1) else (2) end) end) then (case when (aa)<1 then (case when (aa)<1 then (1) else (2) end) else (2) end) else (3) end;
end
begin
bb=case when (aa)<(case when (aa)<1 then (1) else (case when (aa)<1 then (1) else (2) end) end) then (case when (aa)<1 then (case when (aa)<1 then (1) else (2) end) else (2) end) else (3) end;
end
begin
bb=case when (aa)<(case when (aa)<1 then (1) else (case when (aa)<1 then (1) else (2) end) end) then (case when (aa)<1 then (case when (aa)<1 then (1) else (2) end) else (2) end) else (3) end;
end
begin
bb=case when (case when (case when (aa)<(case when (aa)<1 then (1) else (case when (aa)<1 then (1) else (2) end) end) then (case when (aa)<1 then (case when (aa)<1 then (1) else (2) end) else (2) end) else (3) end)<(case when (aa)<1 then (1) else (case when (aa)<1 then (1) else (2) end) end) then (case when (aa)<1 then (case when (aa)<1 then (1) else (2) end) else (2) end) else (3) end)<(case when (aa)<1 then (1) else (case when (aa)<1 then (1) else (2) end) end) then (case when (aa)<1 then (case when (aa)<1 then (1) else (2) end) else (2) end) else (3) end;
end
declare variable v1 char(32) character set win1251;
declare variable v2 varchar(64) character set win1251;
begin
end
end
|
What you mean? |
Hello.
Test case :) |
Do you found a problem or no? |
Hello,
No, I don't have. It was example for testing your new feature. If it can be process such SQL without any problems, I'm sorry. |
It give error about inexistent package, so it worked. |
acf1a4d
to
6a20929
Compare
Wasn't it merged too fast? I hoped to have it reviewed more closely. |
Debug build throws immediately at start of isql: "cannot decrement string_view iterator before begin" Call stack:
|
It would be nice to see hints about this feature in:
|
PS. |
Should be fixed. |
It was discussed in devel, and this implementation is much simpler (but still as discussed) after fact that I give up to treat multibyte alternate strings (say So maybe the merge was fast, but we have no standard here. We have features for many times with opened PR and not commented. We have features commented and stuck with bugs and merged, etc. |
Some strange case encountered with affecting of semicolon that is specified within single-lines comments ("-- ").
This script output is:
Please note on two places:
If I remove ";3" from "-- 1;2;3" then script will go further but again fails on next similar line, which is outside of table definition. Output will be:
And, if we remove ";e" from second line, then script finishes OK. This case encountered during QA auxiliary script execution which failed because of that. |
Fixed, thanks. |
There will be a difference in tests. v5:
master:
Before this change, ISQL eats leading comments and do not pass them to the engine. IMO, this is an error and the new behavior is much more logical. |
Another change was the "inside comment indicator", that I extended one character to align correctly: v5:
master:
|
Instead of this:
i see (on WINDOWS!) a bit different:
i.e. |
This also differs on Windows (and i've sent recently small report by email).
Then let's run:
|
I've checked just now snapshot 6.0.0.142 from github Actions, with timestamp: 09:00 Result: it seems perfect now :-)
-- produces zero output
(i.e. no output)
-- produces:
(i.e. no noise characters/tokens) |
Hello Adriano, Could you test another SQL with '\r' char? This is a C-string:
My results (through IBP) are: Thanks. |
I changed isql and looks like you are reporting differences unrelated to isql. I'm avoiding to give an opinion which is correct now because I'm sure will appear one or two to be contrary. :) |
I asked because I had not seen a test for single '\r' in: BOOST_AUTO_TEST_CASE(SkipSingleLineCommentsTest)
{
FrontendLexer lexer(
"-- comment 0\r\n"
"set -- comment 1\n"
"stats -- comment 2\r\n"
"- -- comment 3\n"
"-- comment 4"
); I think this case should be tested here (in this patch). |
I have following results for case when .sql contains only CR (i.e. ASCII_CHAR(13)) for EOL:
NOTE-1: 6.x does not issue error message, but this is the only branch which reports about affected records (all others "swallow" it) Script is in attached .zip |
@pavel-zotov And on Linux your test shows no error and "Records affected: 0" in all FB versions ;-) |
The sooner isql behavour on Windows will be as on Linux - the better for [almost] all of us ;-) |
Unfortunately, we still have problems with some tests from QA. And all of them seem to be related to new ISQL feature. |
PS. And this also looks strange:
(i mean: why we have to put semicolon at the end of any comment - either multi-line or single-lined ? |
All the problems seems for me as clear bugs of ISQL (before the change). They have no logic for me. If we can't fix this things in major versions, then we can't improve anything. I see no reason to adapt code to accept these cases. |
OK, but is it documented somewhere that one can not specify TWO delimiters without any command between them ?
or
-- ? |
PS And what about #7868 (comment) ? |
- This option can also be activated with command line parameter -autot(erm) | ||
- It can only be used with Firebird engine/server v6 or later | ||
- SET TERM command automatically sets AUTOTERM to OFF | ||
- SET AUTOTERM ON command automatically sets TERM to semicolon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What terminator will be set after SET AUTOTERM OFF ?
Will it remains semicolon or will be returned to the state as before SET AUTOTERM ON ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put in the docs that it is set to semicolon. For me it was clear that it's not reverted when AUTOTERM is turned OFF, but we may improve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was about a bit more clear docs.
Yes, it is so:
Output:
(i.e. no errors)
This is not so (or at least i could not achieve such result):
Output:
|
I will review this case as well the one with comments preceeding the semicolon with an empty statement. But this one is old bug for me:
|
What exactly you consider a bug in this example? Is it in the tracker? |
I didn't knew that |
No, SET TERM must raise an error if new terminator is not given/empty but it doesn't. Did you put this bug into the tracker? |
The code is clear, see |
@pavel-zotov the cases you reported (except things about '\r') should be fixed. |
There is an error here, it should be
|
One more example. Consider script:
Run it. Outcome will include comments, multi- and single- lined (as they are part of executed statements):
|
It's not difficult to change it, but I question. Comments are valid part of a statement text. Saying that engine don't need to parse them to avoid cycles is nonsense for me. Not sending them has consequences in trace and monitoring, for example. |
Yes, in a statement like this:
-- we must see such comments in the trace and mon$statements. But not when comments are placed between statements (like in previous example). |
Which one? Below, above or both? |
Most of examples which caused different output/outcome in v 6.x have been added in QA, test name: Example with comments that are unexpectedly shown ( #7868 (comment) ) not yet added. Waiting for fix / reply. |
No description provided.