You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Jason Dere <jd...@hortonworks.com> on 2013/12/20 00:19:07 UTC
Review Request 16403: HIVE-5176: Wincompat : Changes for allowing various
path compatibilities with Windows
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16403/
-----------------------------------------------------------
Review request for hive and Ashutosh Chauhan.
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
-----
cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java f08a8b6
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java fa3e048
common/src/test/org/apache/hadoop/hive/conf/TestHiveConf.java a31238b
ql/src/java/org/apache/hadoop/hive/ql/exec/CopyTask.java 38d97e3
ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 5cb492f
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 9afc80b
ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java d0be73e
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java b9cd65c
ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 0684aac
ql/src/test/org/apache/hadoop/hive/ql/WindowsPathUtil.java PRE-CREATION
ql/src/test/org/apache/hadoop/hive/ql/exec/TestExecDriver.java d4ad931
ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHiveMetaStoreChecker.java 69d1896
Diff: https://reviews.apache.org/r/16403/diff/
Testing
-------
Thanks,
Jason Dere
Re: Review Request 16403: HIVE-5176: Wincompat : Changes for allowing
various path compatibilities with Windows
Posted by Xuefu Zhang <xz...@cloudera.com>.
> 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
>
>
Re: Review Request 16403: HIVE-5176: Wincompat : Changes for allowing
various path compatibilities with Windows
Posted by Jason Dere <jd...@hortonworks.com>.
> 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().
> >
> >
> >
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?
- Jason
-----------------------------------------------------------
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
>
>
Re: Review Request 16403: HIVE-5176: Wincompat : Changes for allowing
various path compatibilities with Windows
Posted by Xuefu Zhang <xz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16403/#review39269
-----------------------------------------------------------
ql/src/test/org/apache/hadoop/hive/ql/WindowsPathUtil.java
<https://reviews.apache.org/r/16403/#comment71629>
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().
- Xuefu Zhang
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
>
>
Re: Review Request 16403: HIVE-5176: Wincompat : Changes for allowing
various path compatibilities with Windows
Posted by Jason Dere <jd...@hortonworks.com>.
> On April 2, 2014, 7:22 a.m., Thejas Nair wrote:
> > common/src/test/org/apache/hadoop/hive/conf/TestHiveConf.java, line 38
> > <https://reviews.apache.org/r/16403/diff/2/?file=544942#file544942line38>
> >
> > should we just lowercase before comparison, only in windows ?
> >
> >
Sure, will make that change.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16403/#review39243
-----------------------------------------------------------
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
>
>
Re: Review Request 16403: HIVE-5176: Wincompat : Changes for allowing
various path compatibilities with Windows
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16403/#review39243
-----------------------------------------------------------
common/src/test/org/apache/hadoop/hive/conf/TestHiveConf.java
<https://reviews.apache.org/r/16403/#comment71604>
should we just lowercase before comparison, only in windows ?
- Thejas Nair
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
>
>
Re: Review Request 16403: HIVE-5176: Wincompat : Changes for allowing
various path compatibilities with Windows
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16403/#review39485
-----------------------------------------------------------
Ship it!
Ship It!
- Thejas Nair
On April 3, 2014, 1:31 a.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16403/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:31 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 91efb58
> ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHiveMetaStoreChecker.java e453f56
>
> Diff: https://reviews.apache.org/r/16403/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 16403: HIVE-5176: Wincompat : Changes for allowing
various path compatibilities with Windows
Posted by Jason Dere <jd...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16403/
-----------------------------------------------------------
(Updated April 3, 2014, 1:31 a.m.)
Review request for hive and Thejas Nair.
Changes
-------
Changes per review comments.
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 (updated)
-----
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 91efb58
ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHiveMetaStoreChecker.java e453f56
Diff: https://reviews.apache.org/r/16403/diff/
Testing
-------
Thanks,
Jason Dere
Re: Review Request 16403: HIVE-5176: Wincompat : Changes for allowing
various path compatibilities with Windows
Posted by Jason Dere <jd...@hortonworks.com>.
-----------------------------------------------------------
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.
Changes
-------
Moved some functionality out to a different patch.
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 (updated)
-----
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