ngsg commented on code in PR #6013:
URL: https://github.com/apache/hive/pull/6013#discussion_r2431123112
##########
common/src/java/org/apache/hadoop/hive/common/FileUtils.java:
##########
@@ -173,15 +173,15 @@ public static String makePartName(List<String> partCols,
List<String> vals) {
* @return An escaped, valid partition name.
*/
public static String makePartName(List<String> partCols, List<String> vals,
- String defaultStr) {
+ String defaultStr, String defaultPartitionName) {
StringBuilder name = new StringBuilder();
for (int i = 0; i < partCols.size(); i++) {
if (i > 0) {
name.append(Path.SEPARATOR);
}
- name.append(escapePathName((partCols.get(i)).toLowerCase(), defaultStr));
+ name.append(escapePathName((partCols.get(i)).toLowerCase(), defaultStr,
defaultPartitionName));
Review Comment:
Since it is a column name, I think we don't need to pass default partition
name, which cannot be a column name.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -882,7 +880,7 @@ public List<FileStatus>
getFileStatusesForUnpartitionedTable(Database db, Table
* @throws MetaException
*/
public static String makePartName(List<FieldSchema> partCols,
- List<String> vals, String defaultStr) throws MetaException {
+ List<String> vals, String defaultStr, String defaultPartitionName)
throws MetaException {
Review Comment:
Could we unify `defaultStr` and `defaultPartitionName` as suggested in
`FileUtils#escapePathName`?
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/DynamicPartitionCtx.java:
##########
@@ -101,14 +103,14 @@ public DynamicPartitionCtx(List<String> partColNames,
String defaultPartName,
this.customSortNullOrder = new LinkedList<>();
}
- public DynamicPartitionCtx(Map<String, String> partSpec, String
defaultPartName,
Review Comment:
Could you revert the changes here? It seems that the constructor can be used
as-is.
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/BucketMapJoinContext.java:
##########
@@ -207,31 +209,32 @@ public String getMappingBigFile(String alias, String
smallFile) {
// returns fileId for SMBJoin, which consists part of result file name
// needed to avoid file name conflict when big table is partitioned
- public String createFileId(String inputPath) {
+ public String createFileId(String inputPath, Configuration conf) {
String bucketNum = String.valueOf(bucketFileNameMapping.get(inputPath));
if (bigTablePartSpecToFileMapping != null) {
// partSpecToFileMapping is null if big table is partitioned
- return prependPartSpec(inputPath, bucketNum);
+ return prependPartSpec(inputPath, bucketNum, conf);
}
return bucketNum;
}
// returns name of hashfile made by HASHTABLESINK which is read by MAPJOIN
- public String createFileName(String inputPath, String fileName) {
+ public String createFileName(String inputPath, String fileName,
Configuration conf) {
if (bigTablePartSpecToFileMapping != null) {
// partSpecToFileMapping is null if big table is partitioned
- return prependPartSpec(inputPath, fileName);
+ return prependPartSpec(inputPath, fileName, conf);
}
return fileName;
}
// prepends partition spec of input path to candidate file name
- private String prependPartSpec(String inputPath, String fileName) {
+ private String prependPartSpec(String inputPath, String fileName,
Configuration conf) {
Map<String, String> mapping = inputToPartSpecMapping == null ?
inputToPartSpecMapping = revert(bigTablePartSpecToFileMapping) :
inputToPartSpecMapping;
String partSpec = mapping.get(URI.create(inputPath).getPath());
+ // if partSpec is not null or not empty, tableParams and conf can be null
return partSpec == null || partSpec.isEmpty() ? fileName :
- "(" + FileUtils.escapePathName(partSpec) + ")" + fileName;
+ "(" + FileUtils.escapePathName(partSpec,
MetaStoreUtils.getDefaultPartitionName( null, conf)) + ")" + fileName;
Review Comment:
Could you verify the logic here? It looks like we are prepending an escaped
`partSpec` if `partSpec` is not null and nonempty. Then can we safely use
`FileUtils.escapePathName` without default partition name?
##########
ql/src/test/org/apache/hadoop/hive/ql/exec/TestFileSinkOperator.java:
##########
@@ -401,7 +402,9 @@ private FileSinkOperator getFileSink(AcidUtils.Operation
writeType,
partCols.add(new ExprNodeColumnDesc(TypeInfoFactory.stringTypeInfo,
PARTCOL_NAME, "a", true));
Map<String, String> partColMap= new LinkedHashMap<String, String>(1);
partColMap.put(PARTCOL_NAME, null);
- DynamicPartitionCtx dpCtx = new DynamicPartitionCtx(partColMap,
"Sunday", 100);
+ HiveConf conf = new HiveConf();
+ conf.set(HiveConf.ConfVars.DEFAULT_PARTITION_NAME.varname, "Sunday");
+ DynamicPartitionCtx dpCtx = new DynamicPartitionCtx(partColMap, 100,
MetaStoreUtils.getDefaultPartitionName(null, conf));
Review Comment:
Could we use this test as-is? Since the test is not focused on default
partition name, I think it is OK to directly pass "Sunday".
##########
common/src/java/org/apache/hadoop/hive/common/FileUtils.java:
##########
@@ -214,15 +214,15 @@ public static String
makeDefaultListBucketingDirName(List<String> skewedCols,
* @param vals The skewed values
* @return An escaped, valid list bucketing directory name.
*/
- public static String makeListBucketingDirName(List<String> lbCols,
List<String> vals) {
+ public static String makeListBucketingDirName(List<String> lbCols,
List<String> vals, String defaultPartitionName) {
StringBuilder name = new StringBuilder();
for (int i = 0; i < lbCols.size(); i++) {
if (i > 0) {
name.append(Path.SEPARATOR);
}
- name.append(escapePathName((lbCols.get(i)).toLowerCase()));
+ name.append(escapePathName((lbCols.get(i)).toLowerCase(),
defaultPartitionName));
Review Comment:
The same as above, we don't need default partition name for column name.
##########
common/src/java/org/apache/hadoop/hive/common/FileUtils.java:
##########
@@ -274,8 +274,8 @@ static boolean needsEscaping(char c) {
return c < charToEscape.size() && charToEscape.get(c);
}
- public static String escapePathName(String path) {
- return escapePathName(path, null);
+ public static String escapePathName(String path, String
defaultPartitionName) {
Review Comment:
Could we change the two `escapePathName` as follows:
```suggestion
public static String escapePathName(String path, String
defaultPartitionName) {
if (path == null || path.isEmpty()) {
Preconditions.checkArgument(defaultPartitionName != null &&
!defaultPartitionName.isEmpty());
return escapePathName(defaultPartitionName);
} else {
return escapePathName(path);
}
}
/**
* Escapes a path name.
* @param path The path to escape.
* @return An escaped path name.
*/
public static String escapePathName(String path) {
Preconditions.checkArgument(path != null && !path.isEmpty());
// Fast-path detection, no escaping and therefore no copying necessary
int firstEscapeIndex = -1;
```
##########
hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileOutputCommitterContainer.java:
##########
@@ -453,11 +458,12 @@ private Map<String, String>
getStorerParameterMap(StorerInfo storer) {
return params;
}
- private Path constructPartialPartPath(Path partialPath, String partKey,
Map<String, String> partKVs) {
+ private Path constructPartialPartPath(Path partialPath, String partKey,
Map<String, String> partKVs,
+ String defaultPartitionName) {
- StringBuilder sb = new StringBuilder(FileUtils.escapePathName(partKey));
+ StringBuilder sb = new StringBuilder(FileUtils.escapePathName(partKey,
defaultPartitionName));
Review Comment:
This is a column name, so it would be better to use `escapePathName` without
`defaultPartitionName`.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/FileUtils.java:
##########
@@ -299,15 +300,15 @@ public static String escapePathName(String path) {
* The default name for the path, if the given path is empty or
null.
* @return An escaped path name.
*/
- public static String escapePathName(String path, String defaultPath) {
+ public static String escapePathName(String path, String defaultPath, String
defaultPartitionName) {
Review Comment:
Could we update the methods as I commented in `hive.common.FileUtils`?
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -556,8 +556,8 @@ public static String makePartPath(Map<String, String> spec)
* @return partition name
* @throws MetaException
*/
- public static String makePartNameUtil(Map<String, String> spec, boolean
addTrailingSeperator, boolean dynamic)
- throws MetaException {
+ public static String makePartNameUtil(Map<String, String> spec, boolean
addTrailingSeperator, boolean dynamic,
+ String defaultPartitionName) throws MetaException {
Review Comment:
Could we keep the previous method definition? Since the method body stops or
throws an error in case `e.getValue()` is null or empty, I think we don't need
`defaultPartitionName` in both `escapePathName` calls.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java:
##########
@@ -823,8 +821,8 @@ public boolean isDir(Path f) throws MetaException {
}
public static String makePartName(List<FieldSchema> partCols,
- List<String> vals) throws MetaException {
- return makePartName(partCols, vals, null);
+ List<String> vals, String defaultPartitionName) throws MetaException {
Review Comment:
Could we remove this method and force the callers to use the below method?
--
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]
