You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "tprelle (via GitHub)" <gi...@apache.org> on 2023/01/27 10:06:11 UTC
[GitHub] [iceberg] tprelle opened a new pull request, #6676: Nessie : use default namespace location if exists
tprelle opened a new pull request, #6676:
URL: https://github.com/apache/iceberg/pull/6676
In order to mimic HMS behavior, if the namespace of nessie have a location, the table should be store into this location.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6676: Nessie : use default namespace location if exists
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6676:
URL: https://github.com/apache/iceberg/pull/6676#discussion_r1088930835
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -36,6 +36,8 @@
public abstract class BaseMetastoreCatalog implements Catalog {
private static final Logger LOG = LoggerFactory.getLogger(BaseMetastoreCatalog.class);
+ protected static final String NAMESPACE_LOCATION_PROPS = "location";
Review Comment:
why not just keep this in the `NessieCatalog`?
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -202,7 +202,17 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
protected String defaultWarehouseLocation(TableIdentifier table) {
String location;
if (table.hasNamespace()) {
- location = SLASH.join(warehouseLocation, table.namespace().toString(), table.name());
+ String baseLocation;
Review Comment:
maybe define the base location here directly? `String baseLocation = SLASH.join(warehouseLocation, table.namespace().toString())`.
Then you could use it directly in as the fallback: `loadNamespaceMetadata(table.namespace()).getOrDefault(NAMESPACE_LOCATION_PROPS, baseLocation)`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6676: Nessie : use default namespace location if exists
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6676:
URL: https://github.com/apache/iceberg/pull/6676#discussion_r1089009242
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -65,6 +65,7 @@ public class NessieCatalog extends BaseMetastoreCatalog
private static final Logger LOG = LoggerFactory.getLogger(NessieCatalog.class);
private static final Joiner SLASH = Joiner.on("/");
+ public static final String NAMESPACE_LOCATION_PROPS = "location";
Review Comment:
it doesn't look like this is used outside of this class, so let's make this `private`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] tprelle commented on a diff in pull request #6676: Nessie : use default namespace location if exists
Posted by "tprelle (via GitHub)" <gi...@apache.org>.
tprelle commented on code in PR #6676:
URL: https://github.com/apache/iceberg/pull/6676#discussion_r1089000936
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -36,6 +36,8 @@
public abstract class BaseMetastoreCatalog implements Catalog {
private static final Logger LOG = LoggerFactory.getLogger(BaseMetastoreCatalog.class);
+ protected static final String NAMESPACE_LOCATION_PROPS = "location";
Review Comment:
just put it back in the NessieCatalog
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko merged pull request #6676: Nessie : use default namespace location if exists
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #6676:
URL: https://github.com/apache/iceberg/pull/6676
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6676: Nessie : use default namespace location if exists
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6676:
URL: https://github.com/apache/iceberg/pull/6676#discussion_r1088873536
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -202,7 +202,16 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
protected String defaultWarehouseLocation(TableIdentifier table) {
String location;
if (table.hasNamespace()) {
- location = SLASH.join(warehouseLocation, table.namespace().toString(), table.name());
+ String baseLocation;
+ try {
+ baseLocation =
+ loadNamespaceMetadata(table.namespace())
+ .getOrDefault(
+ "location", SLASH.join(warehouseLocation, table.namespace().toString()));
Review Comment:
Thanks for the PR.
How does the user know he needs to configure the namespace property as "location"? hive calls it a "database.locationUri" I guess.
I think we need to standardize it for all the catalogs or document the supported namespace properties per catalog.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] tprelle commented on a diff in pull request #6676: Nessie : use default namespace location if exists
Posted by "tprelle (via GitHub)" <gi...@apache.org>.
tprelle commented on code in PR #6676:
URL: https://github.com/apache/iceberg/pull/6676#discussion_r1088910905
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -202,7 +202,16 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
protected String defaultWarehouseLocation(TableIdentifier table) {
String location;
if (table.hasNamespace()) {
- location = SLASH.join(warehouseLocation, table.namespace().toString(), table.name());
+ String baseLocation;
+ try {
+ baseLocation =
+ loadNamespaceMetadata(table.namespace())
+ .getOrDefault(
+ "location", SLASH.join(warehouseLocation, table.namespace().toString()));
Review Comment:
@ajantha-bhat, i ask myself the same question but as in multiple files in iceberg code (table metadata parser, flink, spark, jdbc, etc...) "location" is used and not a standardize variable.
I can not use spark or hive one because i do not want to add spark or hive dependency to iceberg-nessie.
If the namespace it's created using flink, spark or hive at least like 'CREATE namespace t LOCATION 'somewhere'' it will populate this "location" property.
I just push a new version where i put the static variable on the BaseMetastoreCatalog
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #6676: Nessie : use default namespace location if exists
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #6676:
URL: https://github.com/apache/iceberg/pull/6676#issuecomment-1406394129
cc: @snazy, @dimas-b
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #6676: Nessie : use default namespace location if exists
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6676:
URL: https://github.com/apache/iceberg/pull/6676#discussion_r1089010582
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -202,7 +202,16 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
protected String defaultWarehouseLocation(TableIdentifier table) {
String location;
if (table.hasNamespace()) {
- location = SLASH.join(warehouseLocation, table.namespace().toString(), table.name());
+ String baseLocation;
+ try {
+ baseLocation =
+ loadNamespaceMetadata(table.namespace())
+ .getOrDefault(
+ "location", SLASH.join(warehouseLocation, table.namespace().toString()));
Review Comment:
> If the namespace it's created using flink, spark or hive at least like 'CREATE namespace t LOCATION 'somewhere'' it will populate this "location" property.
Thanks. Then it is already standardized. I was not aware of this (I assumed the user has to manually configure the property bag). I agree with your code changes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] tprelle commented on a diff in pull request #6676: Nessie : use default namespace location if exists
Posted by "tprelle (via GitHub)" <gi...@apache.org>.
tprelle commented on code in PR #6676:
URL: https://github.com/apache/iceberg/pull/6676#discussion_r1089001755
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -202,7 +202,17 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
protected String defaultWarehouseLocation(TableIdentifier table) {
String location;
if (table.hasNamespace()) {
- location = SLASH.join(warehouseLocation, table.namespace().toString(), table.name());
+ String baseLocation;
Review Comment:
I more use to code in scala so without reaffecting the variable,
But i change the code.
Thanks for the review
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] tprelle commented on a diff in pull request #6676: Nessie : use default namespace location if exists
Posted by "tprelle (via GitHub)" <gi...@apache.org>.
tprelle commented on code in PR #6676:
URL: https://github.com/apache/iceberg/pull/6676#discussion_r1089016971
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -65,6 +65,7 @@ public class NessieCatalog extends BaseMetastoreCatalog
private static final Logger LOG = LoggerFactory.getLogger(NessieCatalog.class);
private static final Joiner SLASH = Joiner.on("/");
+ public static final String NAMESPACE_LOCATION_PROPS = "location";
Review Comment:
done
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org