”/* —— */” is official block comment and quite widely used across many programming platforms. Adding it as standard to Fluid Shell would be very helpful.
118 KB
33 KB
47 B
7 KB
Hi TOm,
I have uploaded a the test files the user sent me.
Thank you,
Alonso
Hi TOm,
I have uploaded a the test files the user sent me.
Thank you,
Alonso
The fluidshell command interpreter translates each line at a time. Based on the original design here, the comment delimiter is a # so each line of comment will need to start with a # like
# This is a
# test
<SQL Command>
If the # is not seen, fluidshell will think that /* is a command and pass it to the server. Since that is the case, some comments are acceptable to the server like -- and some are not like /* when passed as an SQL command.
We will need to change ShellLineInterpreter->interpreLinetype to identify block comments within a single line. I don't think it is possible to handle multi-line block comments with this interpreter. We should also add an option in Options->FluidShell to turn this off and on so we don't affect the current behavior for some users.
That will be a start...
Remember that there are three command line interfaces - sql, fluid and shell. All three interfaces need to be tested with this change.
Tom
The fluidshell command interpreter translates each line at a time. Based on the original design here, the comment delimiter is a # so each line of comment will need to start with a # like
# This is a
# test
<SQL Command>
If the # is not seen, fluidshell will think that /* is a command and pass it to the server. Since that is the case, some comments are acceptable to the server like -- and some are not like /* when passed as an SQL command.
We will need to change ShellLineInterpreter->interpreLinetype to identify block comments within a single line. I don't think it is possible to handle multi-line block comments with this interpreter. We should also add an option in Options->FluidShell to turn this off and on so we don't affect the current behavior for some users.
That will be a start...
Remember that there are three command line interfaces - sql, fluid and shell. All three interfaces need to be tested with this change.
Tom
Hi Rajat,
A couple of comments about your check in's...
This line of code below doesn't work because the property doesn't seem to get loaded at startup. I do see it in the properties file. You should also use the constant that you set for the property name rather than hard coding it. It really should be set in one place.
if(cli.conn.getConnectionProperties().getProperty("property.shell.multilinecommentexec","null").equals("true")){
For your method public static String removemultiLineComment(String text) {
since you split on blanks, what happens to something like this? /*select * from "bistudio_example" LIMIT 10
Also, why are you popping s2 or even pushing it since you are throwing away the comments?
Since your method interprets one line at a time setting a local variable like boolean startCommentFlag will not work as the variable is blown away before the next line.
Your code is in the method interpretLineType but you are not setting the line type, Should you?
Thanks, Tom
Hi Rajat,
A couple of comments about your check in's...
This line of code below doesn't work because the property doesn't seem to get loaded at startup. I do see it in the properties file. You should also use the constant that you set for the property name rather than hard coding it. It really should be set in one place.
if(cli.conn.getConnectionProperties().getProperty("property.shell.multilinecommentexec","null").equals("true")){
For your method public static String removemultiLineComment(String text) {
since you split on blanks, what happens to something like this? /*select * from "bistudio_example" LIMIT 10
Also, why are you popping s2 or even pushing it since you are throwing away the comments?
Since your method interprets one line at a time setting a local variable like boolean startCommentFlag will not work as the variable is blown away before the next line.
Your code is in the method interpretLineType but you are not setting the line type, Should you?
Thanks, Tom
Hello Tom,
1. For setting up the property , it will load at the start time and it will be enable/disable via option->fluidshell->execute multi line comment . since we need to register server again to reflect these changes.
That property did not load for me even after stopping and starting ADS hence the if statement did not trigger.
2. As i split on blanks, what happens to something like this? /*select * from "bistudio_example" LIMIT 10
since # comment which is already implemented works same i.e. need a space after it, similarly /* need space for a comment to be act.
I think that # is different as you can have names with a # so the code has to make that distinction. For /* */ , that is not the case. What happens if the user does do /*select *... ? Are you going to pass that as a command?
3. Why s2 is used to push and pop because lets say we have text like " echo /* rajat " i.e. no closing */ then it that case it need to consider /* rajat as this is not in multi line comment.
The method will overwrite s2 with s1 since the insert has the same index??
Hello Tom,
1. For setting up the property , it will load at the start time and it will be enable/disable via option->fluidshell->execute multi line comment . since we need to register server again to reflect these changes.
That property did not load for me even after stopping and starting ADS hence the if statement did not trigger.
2. As i split on blanks, what happens to something like this? /*select * from "bistudio_example" LIMIT 10
since # comment which is already implemented works same i.e. need a space after it, similarly /* need space for a comment to be act.
I think that # is different as you can have names with a # so the code has to make that distinction. For /* */ , that is not the case. What happens if the user does do /*select *... ? Are you going to pass that as a command?
3. Why s2 is used to push and pop because lets say we have text like " echo /* rajat " i.e. no closing */ then it that case it need to consider /* rajat as this is not in multi line comment.
The method will overwrite s2 with s1 since the insert has the same index??
Hello,
As i split on blanks, what happens to something like this? /*select * from "bistudio_example" LIMIT 10
It pass as an command , not treated as a comment
Hello,
As i split on blanks, what happens to something like this? /*select * from "bistudio_example" LIMIT 10
It pass as an command , not treated as a comment
The current behavior is incorrect as Tom noted. Please make the following fixes:
ls /*A multi-line Comment*/ select * from t1 go
Note that /*...*/ inside a quoted string (single or double quotes) should NOT be treated as comments. For example: select * from t1 where C1='This is /*not comment*/'
Also note that the quote character are escaped by a sequence of 2 single or double quote characters. For example: select * from t1 where C1='This is ''/*not comment*/'''
The where clause matches the string: This is '/*not comment*/'
The current behavior is incorrect as Tom noted. Please make the following fixes:
ls /*A multi-line Comment*/ select * from t1 go
Note that /*...*/ inside a quoted string (single or double quotes) should NOT be treated as comments. For example: select * from t1 where C1='This is /*not comment*/'
Also note that the quote character are escaped by a sequence of 2 single or double quote characters. For example: select * from t1 where C1='This is ''/*not comment*/'''
The where clause matches the string: This is '/*not comment*/'
Hello,
following is the fluidshell output :
Hello,
following is the fluidshell output :
Hi Rajat,
An observation...
Hi Rajat,
An observation...
@Rajat:
Please remove the global "Enable Multi-line Comment" setting, which is in File > Options > FluidShell. Having the same setting in global and in Server Properties is confusing to the user because changing the global setting never takes effect on existing registered servers. When a user registers a new server, the "Enable Multi-line Comment" should default to "false".
I am not sure why you need to use a stack in your removemultiLineComment() implementation. It makes the code less efficient and more complicated than necessary. Please consider the following implementation approach:
public static String removeMultiLineComment(CommandLineInterpreter cli, String text) { StringBuilder finalText = new StringBuilder(); int i = 0; while (i < text.length() - 1) { if (!cli.isMultiLineFlagstarts() && text.charAt(i)=='/' && text.charAt(i+1) == '*') { cli.setMultiLineFlagstarts(true); i += 2; continue; } if (cli.isMultiLineFlagstarts() && text.charAt(i)=='*' && text.charAt(i+1) == '/') { cli.setMultiLineFlagstarts(false); i += 2; continue; } if (!cli.isMultiLineFlagstarts()) { finalText.append(text.charAt(i)); } i++; } // append the final character if (!cli.isMultiLineFlagstarts() && i < text.length()) { finalText.append(text.charAt(i)); } return finalText.toString().trim(); }
@Rajat:
Please remove the global "Enable Multi-line Comment" setting, which is in File > Options > FluidShell. Having the same setting in global and in Server Properties is confusing to the user because changing the global setting never takes effect on existing registered servers. When a user registers a new server, the "Enable Multi-line Comment" should default to "false".
I am not sure why you need to use a stack in your removemultiLineComment() implementation. It makes the code less efficient and more complicated than necessary. Please consider the following implementation approach:
public static String removeMultiLineComment(CommandLineInterpreter cli, String text) { StringBuilder finalText = new StringBuilder(); int i = 0; while (i < text.length() - 1) { if (!cli.isMultiLineFlagstarts() && text.charAt(i)=='/' && text.charAt(i+1) == '*') { cli.setMultiLineFlagstarts(true); i += 2; continue; } if (cli.isMultiLineFlagstarts() && text.charAt(i)=='*' && text.charAt(i+1) == '/') { cli.setMultiLineFlagstarts(false); i += 2; continue; } if (!cli.isMultiLineFlagstarts()) { finalText.append(text.charAt(i)); } i++; } // append the final character if (!cli.isMultiLineFlagstarts() && i < text.length()) { finalText.append(text.charAt(i)); } return finalText.toString().trim(); }
Hello,
1.Remove the global "Enable Multi-line Comment" setting, which is in File > Options > FluidShell.
2.Add a check to multi line switch for normal sql statement.
Hello,
1.Remove the global "Enable Multi-line Comment" setting, which is in File > Options > FluidShell.
2.Add a check to multi line switch for normal sql statement.
Revision: 56970
Revision: 56970
It's looking better. However there's an issue with handling how quote characters are escaped. Quote characters are handled as follows:
Examples:
Thus the code implementation should probably have just a single state variable with the following states: Normal, CommentStarted, SingleQuoteStarted, DoubleQuoteStarted
When in Normal state, the implementation should check for characters that has special meaning, such as {slash star}, {single quote}, {double quote}, etc.
When in CommentStarted state, the only character it cares about is {star slash}, which ends the CommentStarted state. Nothing else matters.
When in SingleQuoteStarted state, the only character it cares about is the unscaped {single quote} character, which ends the SingleQuoteStarted state. Everything else is just part of the quoted string.
When in DoubleQuoteStarted state, the only character it cares about is the unscaped {double quote} character, which ends the DoubleQuoteStarted state. Everything else is just part of the quoted string.
Please let me know if you have any questions.
It's looking better. However there's an issue with handling how quote characters are escaped. Quote characters are handled as follows:
Examples:
Thus the code implementation should probably have just a single state variable with the following states: Normal, CommentStarted, SingleQuoteStarted, DoubleQuoteStarted
When in Normal state, the implementation should check for characters that has special meaning, such as {slash star}, {single quote}, {double quote}, etc.
When in CommentStarted state, the only character it cares about is {star slash}, which ends the CommentStarted state. Nothing else matters.
When in SingleQuoteStarted state, the only character it cares about is the unscaped {single quote} character, which ends the SingleQuoteStarted state. Everything else is just part of the quoted string.
When in DoubleQuoteStarted state, the only character it cares about is the unscaped {double quote} character, which ends the DoubleQuoteStarted state. Everything else is just part of the quoted string.
Please let me know if you have any questions.
@Rajat: Please suspend the work on this issue and switch focus to other issues for the v20.5 release. We are re-evaluating the overall approach (all commits for this issue so far) to see if this is the correct approach.
@Rajat: Please suspend the work on this issue and switch focus to other issues for the v20.5 release. We are re-evaluating the overall approach (all commits for this issue so far) to see if this is the correct approach.
@Rajat: After further review of the committed implementation, we've determined that the approach is incorrect.
It turns out that the existing code base already supports block comment. It's just that the customer experienced a defect as per the user's scenario in the attached test(1).zip.
Please revert all code changes that were committed for this issue so far. Instead, please consider the approach in the attached Issue_15690.patch file. If there is a conflict applying the patch, the changes to the ShellLineInterpreter.java is as follows:
public static boolean isInlineGO(CommandLineInterpreter cli, String s) { if (s != null) { s = AQStringUtils.trimRight(s); if (s.length() > 0) { char[] lastCh = new char[1]; DequoteUtil.isBoundedString2(s, lastCh); // Get the last char after processing the block comment to avoid the end of block comment '*/' being used as "GO". char last = lastCh[0]; if ( (isForwardSlashGo(cli) && last == '/') || (isAtSignGo(cli) && last == '@') || (isSemiColonGo(cli) && last == ';') ) { return true; } } } return false; }
Note that as mentioned above, the block comment has always been supported, but there was a bug. Also, there's a "Server Side Comment: /* */" setting in File > Options > Scripts. This option is available per database type. Block comment is supported regardless of whether or not this option is checked. When the option is checked, the block comments is passed to the database server. When the option is unchecked, the block comments is consumed in ADS and NOT passed to the database server. Thus be sure to view the SQL Log for this behavior, and be sure to test all scenarios noted in this issue, and the scenarios in the test(1).zip file attachment.
@Rajat: After further review of the committed implementation, we've determined that the approach is incorrect.
It turns out that the existing code base already supports block comment. It's just that the customer experienced a defect as per the user's scenario in the attached test(1).zip.
Please revert all code changes that were committed for this issue so far. Instead, please consider the approach in the attached Issue_15690.patch file. If there is a conflict applying the patch, the changes to the ShellLineInterpreter.java is as follows:
public static boolean isInlineGO(CommandLineInterpreter cli, String s) { if (s != null) { s = AQStringUtils.trimRight(s); if (s.length() > 0) { char[] lastCh = new char[1]; DequoteUtil.isBoundedString2(s, lastCh); // Get the last char after processing the block comment to avoid the end of block comment '*/' being used as "GO". char last = lastCh[0]; if ( (isForwardSlashGo(cli) && last == '/') || (isAtSignGo(cli) && last == '@') || (isSemiColonGo(cli) && last == ';') ) { return true; } } } return false; }
Note that as mentioned above, the block comment has always been supported, but there was a bug. Also, there's a "Server Side Comment: /* */" setting in File > Options > Scripts. This option is available per database type. Block comment is supported regardless of whether or not this option is checked. When the option is checked, the block comments is passed to the database server. When the option is unchecked, the block comments is consumed in ADS and NOT passed to the database server. Thus be sure to view the SQL Log for this behavior, and be sure to test all scenarios noted in this issue, and the scenarios in the test(1).zip file attachment.
@Rajat: Please completely revert all changes that were committed as part of this issue. This includes the following files:
Do SVN history on each file and revert the changes that were made for this issue.
@Rajat: Please completely revert all changes that were committed as part of this issue. This includes the following files:
Do SVN history on each file and revert the changes that were made for this issue.
Hello Nhi,
All the changes are reverted.
Thanks,
Rajat Batra
Hello Nhi,
All the changes are reverted.
Thanks,
Rajat Batra
Revision: 56989
Added the patch changes to the code.
Revision: 56989
Added the patch changes to the code.
Looks fine. Please be sure to perform the needed unit tests, and then mark the issue as Resolved if the test result is satisfactory.
Looks fine. Please be sure to perform the needed unit tests, and then mark the issue as Resolved if the test result is satisfactory.
We have tested the issue on all the three platforms i.e Windows, Mac and Ubuntu. Block comments are working fine across all platforms for ADS 20.5 with all three modes(SQL, Fluid, Shell) within Fluidshell and as well as with OS terminal .
Following are details of components where the issue was tested
Build: v20.5.0-no-ofsc-dev-26
Platforms: Windows 10, Ubuntu 17.04, Mac 10.13
Testcase link for Windows: https://idera.testrail.net/index.php?/runs/view/2190&group_by=cases:section_id&group_order=asc&group_id=13189
We have tested the issue on all the three platforms i.e Windows, Mac and Ubuntu. Block comments are working fine across all platforms for ADS 20.5 with all three modes(SQL, Fluid, Shell) within Fluidshell and as well as with OS terminal .
Following are details of components where the issue was tested
Build: v20.5.0-no-ofsc-dev-26
Platforms: Windows 10, Ubuntu 17.04, Mac 10.13
Testcase link for Windows: https://idera.testrail.net/index.php?/runs/view/2190&group_by=cases:section_id&group_order=asc&group_id=13189
The test run details here:
https://idera.testrail.net/index.php?/runs/view/2412&group_by=cases:section_id&group_order=asc
The test run details here:
https://idera.testrail.net/index.php?/runs/view/2412&group_by=cases:section_id&group_order=asc
QE testing was completed. Please find the details below
QE testing was completed. Please find the details below
Issue #15690 |
Verified |
Fixed |
Resolved |
Completion |
No due date |
Fixed Build v20.5.0-no-ofsc-dev-23 |
No time estimate |
1 issue link |
relates to #6211
Issue #6211FluidShell: how do i specify comments within commands ? |
Hi Alonso,
Did the user give us any examples of where the block comments do not work? I tested here and seems to work for me.
Thanks, Tom