> On April 2, 2014, 4:07 p.m., Xuefu Zhang wrote: > > ql/src/test/org/apache/hadoop/hive/ql/WindowsPathUtil.java, line 27 > > <https://reviews.apache.org/r/16403/diff/2/?file=544943#file544943line27> > > > > For what it's worth, it seems the if (windows) should stay with the > > caller of the method. That is, caller should do: > > > > if( windows) { > > WindowsPathUtil.convertPaths(); > > } > > > > Same for getHdfsUriString(). > > > > > > > > Jason Dere wrote: > For the first point, sure we can move the if (windows) check to > TestExecDriver/TestHiveMetaStoreChecker/any other callers. > For getHdfsUriString() though, this is a private method and it seems like > doing this will just bloat convertPathsFromWindowsToHdfs(). Would you prefer > if we just eliminated that if(windows) check in getHdfsUriString(), given > that calling a method in WindowsPathUtil likely means that the caller is > trying to do Windows path-specific stuff?
Sorry that I didn't notice that getHdfsUriString() is private. So, if(windows) check isn't necessary. My previous point was mainly that the class name suggests windows specific, so it should only handle windows specific things. It's the caller's responsibility to do the if check. The alternative is to rename the class/method to make platform neutral. Either way is fine to me. - Xuefu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16403/#review39269 ----------------------------------------------------------- On April 2, 2014, 2:11 a.m., Jason Dere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16403/ > ----------------------------------------------------------- > > (Updated April 2, 2014, 2:11 a.m.) > > > Review request for hive and Thejas Nair. > > > Bugs: HIVE-5176 > https://issues.apache.org/jira/browse/HIVE-5176 > > > Repository: hive-git > > > Description > ------- > > We need to make certain changes across the board to allow us to read/parse > windows paths. Some are escaping changes, some are being strict about how we > read paths (through URL.encode/decode, etc) > > > Diffs > ----- > > common/src/test/org/apache/hadoop/hive/conf/TestHiveConf.java a31238b > ql/src/test/org/apache/hadoop/hive/ql/WindowsPathUtil.java PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/exec/TestExecDriver.java 5991aae > > ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHiveMetaStoreChecker.java > e453f56 > > Diff: https://reviews.apache.org/r/16403/diff/ > > > Testing > ------- > > > Thanks, > > Jason Dere > >
