aturoczy commented on code in PR #4880:
URL: https://github.com/apache/hive/pull/4880#discussion_r1399117467
##########
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:
##########
@@ -2224,6 +2224,10 @@ public static enum ConfVars {
HIVE_ICEBERG_EXPIRE_SNAPSHOT_NUMTHREADS("hive.iceberg.expire.snapshot.numthreads",
4,
"The number of threads to be used for deleting files during expire
snapshot. If set to 0 or below it uses the" +
" defult DirectExecutorService"),
+
+ HIVE_ICEBERG_MASK_DEFAULT_LOCATION("hive.iceberg.mask.default.location",
false,
+ "If set to true the URI for auth will have the default location masked
with DEFAULT_TABLE_LOCATION"),
Review Comment:
If **this is** set to true
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java:
##########
@@ -212,6 +216,8 @@ public class HiveIcebergStorageHandler implements
HiveStoragePredicateHandler, H
public static final String MERGE_ON_READ = "merge-on-read";
public static final String STATS = "/stats/snap-";
+ public static final String TABLE_DEFAULT_LOCATION = "TABLE_DEFAULT_LOCATION";
Review Comment:
What is the TABLE_DEFAULT_LOCATION here? It would not be a default path?
Would not be /user/hive/warehouse?
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java:
##########
@@ -1038,6 +1050,28 @@ static String encodeString(String rawString) {
return
HiveConf.EncoderDecoderFactory.URL_ENCODER_DECODER.encode(rawString);
}
+ static String getLocation(String location, String defaultLocation,
Configuration conf) {
+ if (location == null || defaultLocation == null) {
+ return location;
+ }
+ boolean maskDefaultLocation =
conf.getBoolean(HiveConf.ConfVars.HIVE_ICEBERG_MASK_DEFAULT_LOCATION.varname,
+ HiveConf.ConfVars.HIVE_ICEBERG_MASK_DEFAULT_LOCATION.defaultBoolVal);
+ try {
+ return maskDefaultLocation ?
+ getQualifiedPath(location,
conf).replaceFirst(getQualifiedPath(defaultLocation, conf),
+ TABLE_DEFAULT_LOCATION) :
+ location;
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ static String getQualifiedPath(String location, Configuration conf) throws
IOException {
+ Path path = new Path(location);
+ FileSystem fs = path.getFileSystem(conf);
Review Comment:
Maybe it is not true for java. But this type of resource should be closable
(disposable) in .net. Aka it should be close the resource. I guess there could
be some mechanism in Java that could close automatically, but even if it has
(don't know) but the best practice is always to close a resource that is not
used anymore. Especially those that is coming from the OS
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -14215,17 +14216,18 @@ ASTNode analyzeCreateTable(
Warehouse wh = new Warehouse(conf);
tblLocation =
wh.getDefaultTablePath(db.getDatabase(qualifiedTabName.getDb()),
qualifiedTabName.getTable(),
isExt).toUri().getPath();
+ SessionStateUtil.addResourceOrThrow(conf, DEFAULT_TABLE_LOCATION,
tblLocation);
} catch (MetaException | HiveException e) {
throw new SemanticException(e);
}
}
try {
HiveStorageHandler storageHandler = HiveUtils.getStorageHandler(conf,
storageFormat.getStorageHandler());
if (storageHandler != null) {
- storageHandler.addResourcesForCreateTable(tblProps, conf);
+ storageHandler.addResourcesForCreateTable(tblProps, conf, db,
qualifiedTabName, isExt);
}
- } catch (HiveException e) {
- throw new RuntimeException(e);
+ } catch (HiveException | MetaException e) {
Review Comment:
Rarely see the pipe in exception :) 👍
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -14215,17 +14216,18 @@ ASTNode analyzeCreateTable(
Warehouse wh = new Warehouse(conf);
tblLocation =
wh.getDefaultTablePath(db.getDatabase(qualifiedTabName.getDb()),
qualifiedTabName.getTable(),
isExt).toUri().getPath();
+ SessionStateUtil.addResourceOrThrow(conf, DEFAULT_TABLE_LOCATION,
tblLocation);
} catch (MetaException | HiveException e) {
throw new SemanticException(e);
}
}
try {
HiveStorageHandler storageHandler = HiveUtils.getStorageHandler(conf,
storageFormat.getStorageHandler());
if (storageHandler != null) {
- storageHandler.addResourcesForCreateTable(tblProps, conf);
+ storageHandler.addResourcesForCreateTable(tblProps, conf, db,
qualifiedTabName, isExt);
}
- } catch (HiveException e) {
- throw new RuntimeException(e);
+ } catch (HiveException | MetaException e) {
Review Comment:
Rarely see the pipe in exception :) 👍
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java:
##########
@@ -1038,6 +1050,28 @@ static String encodeString(String rawString) {
return
HiveConf.EncoderDecoderFactory.URL_ENCODER_DECODER.encode(rawString);
}
+ static String getLocation(String location, String defaultLocation,
Configuration conf) {
+ if (location == null || defaultLocation == null) {
+ return location;
Review Comment:
If location is null or defaultLocation is null would not be better to throw
an exception? Or it's value checked somewhere else?
--
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]
