zabetak commented on code in PR #4476:
URL: https://github.com/apache/hive/pull/4476#discussion_r1260912937
##########
hplsql/src/test/queries/local/dbms_output.sql:
##########
@@ -3,4 +3,7 @@ DECLARE
BEGIN
DBMS_OUTPUT.PUT_LINE('Hello, world!');
DBMS_OUTPUT.PUT_LINE(str);
+ DBMS_OUTPUT.PUT_LINE(trim('a''a'));
+ DBMS_OUTPUT.PUT_LINE(trim('''a'));
+ DBMS_OUTPUT.PUT_LINE(trim('a'''));
Review Comment:
If the `trim` function is not necessary for reproducing the problem then we
should remove it. In general, we should strive to keep repro tests minimal to
facilitate understanding and also avoid side effects and unrelated failures in
the future by the inclusion of extra elements.
##########
itests/hive-unit/pom.xml:
##########
@@ -483,6 +483,12 @@
<version>${htmlunit.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>net.sf.supercsv</groupId>
+ <artifactId>super-csv</artifactId>
+ <version>${super-csv.version}</version>
+ <scope>test</scope>
+ </dependency>
Review Comment:
Why do we need this new dependency here? I don't see how it is relevant to
the problem or changes in the PR.
##########
hplsql/src/test/queries/local/dbms_output.sql:
##########
@@ -3,4 +3,7 @@ DECLARE
BEGIN
DBMS_OUTPUT.PUT_LINE('Hello, world!');
DBMS_OUTPUT.PUT_LINE(str);
+ DBMS_OUTPUT.PUT_LINE(trim('a''a'));
+ DBMS_OUTPUT.PUT_LINE(trim('''a'));
+ DBMS_OUTPUT.PUT_LINE(trim('a'''));
Review Comment:
What should happen in the following cases:
```
DBMS_OUTPUT.PUT_LINE('a'');
DBMS_OUTPUT.PUT_LINE(''a');
```
Should we add these test cases as well?
##########
hplsql/src/test/queries/local/dbms_output.sql:
##########
@@ -3,4 +3,7 @@ DECLARE
BEGIN
DBMS_OUTPUT.PUT_LINE('Hello, world!');
DBMS_OUTPUT.PUT_LINE(str);
+ DBMS_OUTPUT.PUT_LINE(trim('a''a'));
+ DBMS_OUTPUT.PUT_LINE(trim('''a'));
+ DBMS_OUTPUT.PUT_LINE(trim('a'''));
Review Comment:
Please also include the following test cases:
```
'''''aa'
'a''''a'
'aa'''''
```
##########
hplsql/src/main/java/org/apache/hive/hplsql/Utils.java:
##########
@@ -38,11 +38,11 @@ public static String unquoteString(String s) {
char ch = s.charAt(i);
char ch2 = (i < len - 1) ? s.charAt(i+1) : 0;
- if((i == 0 || i == len -1) && (ch == '\'' || ch == '"'))
+ if((i == 0 || i == len - 1) && (ch == '\'' || ch == '"'))
continue;
else
- // \' and '' escape sequences
- if((ch == '\\' && ch2 == '\'') || (ch == '\'' && ch2 == '\''))
+ // \' and '' escape sequences and include ' if last two characters
are ''
+ if((ch == '\\' && ch2 == '\'') || (ch == '\'' && ch2 == '\'' && i
!= len - 2))
Review Comment:
Please create a class `TestUtils` and add unit tests for this method; these
are in general faster and can be made more exhaustive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
