----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57693/#review169650 -----------------------------------------------------------
Thanks for the changes. I think this would be very helpful to be able to use qfiles using BeeLine. beeline/src/java/org/apache/hive/beeline/BeeLine.java Line 1758 (original), 1758 (patched) <https://reviews.apache.org/r/57693/#comment242163> Do you think it would be better to keep this as unmodifiableMap and inject qtest outputformat when BeeLine is used for qtests only? May be we can create a package private BeeLine constructor which is used from QFileBeeLineClient and we pass a flag which is used to inject it here. What do you think? itests/util/src/main/java/org/apache/hive/beeline/QFileOutputFormat.java Lines 62 (patched) <https://reviews.apache.org/r/57693/#comment242160> Can you please add some documentation for this method so that it is easier to understand. It would be great if you could add a sample String[] vals and output String from this method. itests/util/src/main/java/org/apache/hive/beeline/QFileOutputFormat.java Lines 65 (patched) <https://reviews.apache.org/r/57693/#comment242158> Do we need a null check here? itests/util/src/main/java/org/apache/hive/beeline/QFilePostExecutePrinter.java Lines 27 (patched) <https://reviews.apache.org/r/57693/#comment242159> Do we need two (QFilePost/QFilePre ExecutePrinter) separate classes? seems like both are doing the same thing. itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java Lines 78 (patched) <https://reviews.apache.org/r/57693/#comment242162> Not sure I understand this. Can you add some comments so that it is easier to understand? Is it because BOUNDING_CHAR is both at the starting and the ending of the message? ql/src/java/org/apache/hadoop/hive/ql/hooks/PrinterHook.java Lines 31 (patched) <https://reviews.apache.org/r/57693/#comment242161> Does this need to be abstract? - Vihang Karajgaonkar On March 16, 2017, 5:15 p.m., Peter Vary wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57693/ > ----------------------------------------------------------- > > (Updated March 16, 2017, 5:15 p.m.) > > > Review request for hive, Zoltan Haindrich, Marta Kuczora, Miklos Csanady, > Naveen Gangam, Vihang Karajgaonkar, and Barna Zsombor Klara. > > > Bugs: HIVE-16146 > https://issues.apache.org/jira/browse/HIVE-16146 > > > Repository: hive-git > > > Description > ------- > > The goal was to generate '\0' markers around the raw log items we want to > keep in the golden files. > > To archive this I had to do a small functional change and some small refactor: > - Removed the immutability of the format map in BeeLine, so the test could > add the QFile specific OutputFromat as a possible format > - The PostExecutePrinter and the PreExecutePrinter got a common ancestor, > which was due anyway because PostExecutePrinter reused static methods from > PreExecutePrinter. This way I was able to create QFile specific printers > which are generating the desired markers. > - Moved the QFile test to the org.apache.hive.beeline package, so the test > classes can use the package private classes and methods > > For one reason or other BeeLine added an extra space character at the end of > the lines for multiline commands - I have removed this space - Will see if > this effects any unit test or not. > > With the above mentioned *OutputFromat*, *QFilePreExecutePrinter*, > *QFilePostExecutePrinter* we can mark the lines which are needed in the q.out > file, and during the filtering we can remove the unneeded parts - I prefer > to keep the log level high in the raw files, so in case of a test failure we > can have better understanding of what has happened. > > In the test files: > - Updated the beforeExecute, and afterExecute methods to set the new > outputformat and the new hooks > - Removed the query specific filters, since they are non exitstent in the CLI > tests > - Simplified the static filterset - currently only contains the filters which > are really neccessary for the actual tests - might grow to the same size than > in the QTestUtils - but if we do not want to run all of the test we would > like to keep this list as small as possible > - Removed unnecessary configurations from QFileBuilder which will be not > needed in case we want to mimic the CLI results > > > Diffs > ----- > > beeline/src/java/org/apache/hive/beeline/BeeLine.java 3c8fccc > beeline/src/java/org/apache/hive/beeline/Commands.java 99ee82c > itests/src/test/resources/testconfiguration.properties 0c590c8 > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java > acc02eb > itests/util/src/main/java/org/apache/hive/beeline/QFileOutputFormat.java > PRE-CREATION > > itests/util/src/main/java/org/apache/hive/beeline/QFilePostExecutePrinter.java > PRE-CREATION > > itests/util/src/main/java/org/apache/hive/beeline/QFilePreExecutePrinter.java > PRE-CREATION > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java 49d6d24 > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFileBeeLineClient.java > b6eac89 > itests/util/src/main/java/org/apache/hive/beeline/qfile/package-info.java > fcd50ec > ql/src/java/org/apache/hadoop/hive/ql/hooks/PostExecutePrinter.java b4fc125 > ql/src/java/org/apache/hadoop/hive/ql/hooks/PreExecutePrinter.java 232c62d > ql/src/java/org/apache/hadoop/hive/ql/hooks/PrinterHook.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 0732207 > ql/src/test/results/clientpositive/beeline/drop_with_concurrency.q.out > d22c9ec > ql/src/test/results/clientpositive/beeline/escape_comments.q.out 5f9df93 > ql/src/test/results/clientpositive/beeline/select_dummy_source.q.out > PRE-CREATION > > > Diff: https://reviews.apache.org/r/57693/diff/1/ > > > Testing > ------- > > Added a new simple query file from CLI driver, and checked that the generated > output is the same > > > Thanks, > > Peter Vary > >
