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
HIVE-15820: comment at the head of beeline -e #1814
Conversation
The function trim() is called by HiveStringUtils.removeComments(String, int[]). As HiveStringUtils.removeComments(String, int[]) process a multiline statement as a single line string, only the leading spaces in the first line and the trailing spaces in the last line are removed. After I replaced HiveStringUtils.removeComments(String, int[]) with HiveStringUtils.removeComments(String), the leading spaces and trailing spaces in each line are trimmed. It's why 6 tests failed. Rather than change HiveStringUtils.removeComments(String, int[]), I changed the test files as the new behaviour is expected. |
I moved trim() from HiveStringUtils.removeComments(String, int[]) to HiveStringUtils.removeComments(String) so that I only need to change TestHiveStringUtils.java instead of 6 other test result files. In my option, HiveStringUtils.removeComments(String, int[]) shouldn't trim the spaces. Otherwise, we'll lose the indents in a formatted SQL statement. Trimming the leading and trailing spaces of the whole statement should be enough. |
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.
changes look good. I just requested an additional test scenario. Could you also include some of your test output, manually done, in the comments? Thank you
assertEquals("\"show --comments tables\"", removeComments("--comments\n\"show --comments tables\" --comments")); | ||
assertEquals("\"'show --comments' tables\"", removeComments("--comments\n\"'show --comments' tables\" --comments")); | ||
assertEquals("'show --comments tables'", removeComments("--comments\n'show --comments tables' --comments")); | ||
assertEquals("'\"show --comments tables\"'", removeComments("--comments\n'\"show --comments tables\"' --comments")); |
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.
Could you please add a test scenario where a multiline query strings has comments in between fragments of query? just as an example
"select col1,
--partitioned year column
year,
--partitioned month column
month,
--partitioned date column
date
from test_table
where
--for a particular user
username = 'foo';"
should return something equivalent to
"select col1, year, month, date from test_table where username = 'foo';"
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.
Thanks for having a look! I added your example. It contains both separate comment lines and inline comment.
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.
Made the change as you advised. :)
assertEquals("\"show --comments tables\"", removeComments("--comments\n\"show --comments tables\" --comments")); | ||
assertEquals("\"'show --comments' tables\"", removeComments("--comments\n\"'show --comments' tables\" --comments")); | ||
assertEquals("'show --comments tables'", removeComments("--comments\n'show --comments tables' --comments")); | ||
assertEquals("'\"show --comments tables\"'", removeComments("--comments\n'\"show --comments tables\"' --comments")); |
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.
Thanks for having a look! I added your example. It contains both separate comment lines and inline comment.
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.
Thanks for the changes. Change looks good to me. +1 for me.
…ne -e ### _Why are the changes needed?_ Backport apache/hive#1814 related kyuubi issues #4305 #4333 #4406 ### _How was this patch tested?_ - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request Closes #4972 from turboFei/beeline_head_command. Closes #4305 04b235f [fwang12] [KYUUBI #4305][Bug] Backport HIVE-15820: comment at the head of beeline -e Authored-by: fwang12 <fwang12@ebay.com> Signed-off-by: fwang12 <fwang12@ebay.com>
Backport apache/hive#1814 related kyuubi issues - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request Closes #4972 from turboFei/beeline_head_command. Closes #4305 04b235f [fwang12] [KYUUBI #4305][Bug] Backport HIVE-15820: comment at the head of beeline -e Authored-by: fwang12 <fwang12@ebay.com> Signed-off-by: fwang12 <fwang12@ebay.com>
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
No.
How was this patch tested?