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

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

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,

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:

1. In ShellLineInterpreter.java, please replace all instances of "property.shell.multilinecommentexec" with FluidShellConstants.FLUIDSHELL_MULTILINECOMMENTEXEC. We should use a static constant rather than retyping the same string.
2. Replace the "Execute Multi Line Comment" title/description with "Enable Multi-line Comment".
3. The implementation of ShellLineInterpreter::removemultiLineComment() is incorrect. It does not work on multiple lines, nor does it work when there is no space after the "/*". It needs to work on the following input:

   ls /*A
   multi-line
   Comment*/
   select * from t1
   go

   
   It also needs to work when the executing the same content stored in a file (see attached test-comment.txt). For example: source test-comment.txt

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, 

1.  Replace  instances of "property.shell.multilinecommentexec" with FluidShellConstants.FLUIDSHELL_MULTILINECOMMENTEXEC.
2. Replaced the "Execute Multi Line Comment" title/description with "Enable Multi-line Comment".
3. Implementation is updated work on multiple lines also when there is no space after the "/*". 
4. This couple of things still needs to implement : /*...*/ inside a quoted string (single or double quotes) should NOT be treated as comments. and needs to work when the executing the same content stored in a file

following is the fluidshell output :

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();
}

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

1. Added a check for quotes in the text.  /*...*/ inside a quoted string is not treated as comment.

It's looking better. However there's an issue with handling how quote characters are escaped. Quote characters are handled as follows:

1. A quoted string may start with a single quote or double quote character, and MUST end with the SAME un-escaped quote character.
2. Characters inside a quoted string has NO special meaning.
3. A subsequent quote character that is the same as the starting quote character can be escaped by 2 occurrances of the SAME quote character right next to each other.

Examples:

1. {single quote}hello{double quote} there{single quote}
   • This is a quoted string of the following content: hello" there
   • Note the double quote character has no meaning inside the single quoted string
2. {double quote}hello{single quote} there{double quote}
   • This is a quoted string of the following content: hello' there
   • Note the single quote character has no meaning inside the double quoted string
3. {single quote}hello{single quote}{double quote} there{single quote}
   • This is a quoted string of the following content: hello
   • Note that the quoted string is ended by the second single quote character.
   • Note that the double quote character starts a second quoted string with the following content:  there'
   • Note that the second quoted string has NOT ended since we have not encounter another un-escaped double quote character.
   • Note that the {single quote}{double quote} character sequence does NOT escape anything because they are not identical characters.
4. {single quote}hello{double quote}{double quote} there{single quote}
   • This is a quoted string of the following content: hello"" there
   • Note that the {double quote}{double quote} character sequence does NOT constitute an escaped character. Since they are inside a quoted string, they are taken literally as 2 double quote characters.
   • The only character that you can escape is the quote character that started the quoted string, in this case the single quote character.

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: 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:

1. BaseFluidShellPanel.java
2. RunShellScriptOptions.java
3. ShellVariable.java
4. CommandLineInterpreter.java
5. FluidShellConstants.java
6. ShellLineInterpreter.java
7. OptionSettingsDefaults.java
8. OptionSettings.java
9. RunShellScriptOptionVariables.java
10. OptionsText.java

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

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.

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

Ubuntu: https://idera.testrail.net/index.php?/runs/view/2283&group_by=cases:section_id&group_order=asc&group_id=13189

Mac: https://idera.testrail.net/index.php?/runs/view/2284&group_by=cases:section_id&group_order=asc&group_id=13149

Has this been verified by Automation?

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