You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2024/11/21 13:48:48 UTC
[PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
nastra opened a new pull request, #11620:
URL: https://github.com/apache/iceberg/pull/11620
(no comment)
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1860118523
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ if (table instanceof SerializableTable) {
+ return formatVersion(((SerializableTable) table).underlyingTable());
+ } else if (table instanceof PositionDeletesTable) {
Review Comment:
@RussellSpitzer had more or less the same feedback on this, so let's remove this handling for `PositionDeletesTable` from core and do it in the maintenance procedure
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852337171
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
Review Comment:
I would strongly prefer we keep it out of table properties. IMHO, Table properties are user facing hints or guides and the system should not rely on them for correct behavior. Having the correct format version is important for correct behavior so I would keep it out of properties.
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852487389
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // being able to read the format version from the PositionDeletesTable is mainly needed in
+ // SparkPositionDeletesRewrite when determining whether to rewrite V2 position deletes to DVs
+ if (table instanceof BaseMetadataTable) {
Review Comment:
> I would recommend we just go through all of our cases narrower then broader if we have exceptions so
I would prefer that too, but the fact that `SerializableMetadataTable` is a subclass of `SerializableTable` which in turn implements `HasTableOperations` makes this more difficult and you still need to differentiate there
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852542188
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // being able to read the format version from the PositionDeletesTable is mainly needed in
+ // SparkPositionDeletesRewrite when determining whether to rewrite V2 position deletes to DVs
+ if (table instanceof BaseMetadataTable) {
Review Comment:
Yes. But I also don't think the caller should need to check. Shouldn't the caller know what it's doing? Like if it is compacting DeleteVectors it knows it has a DeleteVectorMetadataTable and therefor it uses the parent table.
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1884369789
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -158,6 +160,21 @@ public Map<String, String> properties() {
return properties;
}
+ public int formatVersion() {
+ return formatVersion;
+ }
+
+ private int formatVersion(Table table) {
+ if (table instanceof HasTableOperations) {
+ return ((HasTableOperations) table).operations().current().formatVersion();
Review Comment:
Optional: What about an extra variable for `TableOperations`? We have an explicit casts and 3 method invocations on a single line. An extra variable may be easier to read and would be consistent with `metadataFileLocation`.
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -158,6 +160,21 @@ public Map<String, String> properties() {
return properties;
}
+ public int formatVersion() {
+ return formatVersion;
+ }
+
+ private int formatVersion(Table table) {
+ if (table instanceof HasTableOperations) {
+ return ((HasTableOperations) table).operations().current().formatVersion();
+ } else {
+ // formatVersion=-1 will never be used/returned, because
Review Comment:
Optional: We may actually avoid the override by doing something like below.
```
public int formatVersion() {
if (formatVersion == UNKNOWN_FORMAT_VERSION) {
throw new UnsupportedOperationException("Format version is unknown");
}
return formatVersion;
}
private int formatVersion(Table table) {
if (table instanceof HasTableOperations) {
...
} else {
return UNKNOWN_FORMAT_VERSION;
}
}
```
This would also cover cases for regular tables that didn't have `TableOperations` under the hood.
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852425487
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // being able to read the format version from the PositionDeletesTable is mainly needed in
+ // SparkPositionDeletesRewrite when determining whether to rewrite V2 position deletes to DVs
+ if (table instanceof BaseMetadataTable) {
Review Comment:
> Although I am curious why position delete table needs a format version?
@RussellSpitzer this is mainly for `SparkPositionDeletesRewrite` (which operates against the position deletes table). Basically when rewriting existing position deletes we need to know whether we need to rewrite them to V2 position deletes or to DVs by looking at the underlying format version of the table. A table that was upgraded to V3 can still have V2 position deletes, meaning that these would then have to be rewritten as DVs
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852473169
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -120,7 +120,7 @@ private FileIO fileIO(Table table) {
return table.io();
}
- private Table lazyTable() {
+ protected Table lazyTable() {
Review Comment:
Oh yes, right. I misread. Good to me then.
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1872737219
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -62,13 +62,14 @@ public class SerializableTable implements Table, HasTableOperations, Serializabl
private final FileIO io;
private final EncryptionManager encryption;
private final Map<String, SnapshotRef> refs;
+ private final UUID uuid;
+ private final int formatVersion;
private transient volatile LocationProvider lazyLocationProvider = null;
private transient volatile Table lazyTable = null;
private transient volatile Schema lazySchema = null;
private transient volatile Map<Integer, PartitionSpec> lazySpecs = null;
private transient volatile SortOrder lazySortOrder = null;
- private final UUID uuid;
Review Comment:
moving this up to the other final fields
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #11620:
URL: https://github.com/apache/iceberg/pull/11620
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852350086
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // being able to read the format version from the PositionDeletesTable is mainly needed in
+ // SparkPositionDeletesRewrite when determining whether to rewrite V2 position deletes to DVs
+ if (table instanceof BaseMetadataTable) {
Review Comment:
One of the reasons I want scala is I really want to just enumerate cases here. I would recommend we just go through all of our cases narrower then broader if we have exceptions so
```java
If (PositionDeleteTable)
return format version
else if (MetaTable) {
Sorry Brah
}
else if (HasTableOperations) {
return format version
}
else {
// Sorry Brah
}
```
Although I am curious why position delete table needs a format version?
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852502401
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // being able to read the format version from the PositionDeletesTable is mainly needed in
+ // SparkPositionDeletesRewrite when determining whether to rewrite V2 position deletes to DVs
+ if (table instanceof BaseMetadataTable) {
Review Comment:
> Shouldn't we just be looking at the underlying table then? Shouldn't the converter look at the base table rather than the metadata table?
I'm not fully sure I follow your comment. Do you mean the calling site should first check whether it's a metadata table before calling `TableUtil.formatVersion(...)`?
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1872275033
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /** Returns the format version of the given table */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // SerializableMetadataTable is a subclass of SerializableTable but does not support
+ // operations()
+ if (table instanceof HasTableOperations
Review Comment:
I think we still have a problem that getting `SerializableTable` here would require loading the metadata location and sending a request to the storage. I would consider capturing format version as a field in `SerializableTable` and throwing `UnsupportedOperationException` in `SerializableMetadataTable` that implements it. Then the whole logic can be structured as follows:
```
if (table instanceof SerializableTable) {
SerializableTable serializableTable = (SerializableTable) table;
return serializableTable.formatVersion();
} else if (table instanceof HasTableOperations) {
TableOperations ops = ((HasTableOperations) table).operations();
return ops.current().formatVersion();
} else {
throw new IllegalArgumentException(
String.format("%s does not have a format version", table.getClass().getSimpleName()));
}
```
What do you think, @nastra?
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1872409911
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ if (table instanceof SerializableTable) {
+ return formatVersion(((SerializableTable) table).underlyingTable());
+ } else if (table instanceof PositionDeletesTable) {
Review Comment:
Me and Anton are on the same psychic link
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1859414335
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -143,6 +143,10 @@ protected Table newTable(TableOperations ops, String tableName) {
return new BaseTable(ops, tableName);
}
+ public Table underlyingTable() {
Review Comment:
Question: What about capturing the format version as a field in `SerializableTable`, similar to what we do for the metadata file location? The problem right now is that calling `lazyTable()` may actually require a request to load the metadata, which is something we would want to ideally avoid. Historically, we kept separate fields for what is considered important information and can be accessed frequently.
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1886323591
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -158,6 +160,21 @@ public Map<String, String> properties() {
return properties;
}
+ public int formatVersion() {
+ return formatVersion;
+ }
+
+ private int formatVersion(Table table) {
+ if (table instanceof HasTableOperations) {
+ return ((HasTableOperations) table).operations().current().formatVersion();
+ } else {
+ // formatVersion=-1 will never be used/returned, because
Review Comment:
that makes sense, I've updated this (although I've used a slightly different error msg to be consistent with the error msg in `TableUtil`)
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852176503
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -120,7 +120,7 @@ private FileIO fileIO(Table table) {
return table.io();
}
- private Table lazyTable() {
+ protected Table lazyTable() {
Review Comment:
this method isn't used for test purposes, but rather to get access to the underlying table in the subclass
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852339111
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // being able to read the format version from the PositionDeletesTable is mainly needed in
+ // SparkPositionDeletesRewrite when determining whether to rewrite V2 position deletes to DVs
+ if (table instanceof BaseMetadataTable) {
Review Comment:
I wish we had scala or even kotlin
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1853654808
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // being able to read the format version from the PositionDeletesTable is mainly needed in
+ // SparkPositionDeletesRewrite when determining whether to rewrite V2 position deletes to DVs
+ if (table instanceof BaseMetadataTable) {
Review Comment:
I've simplified the checks a bit. I do think it makes sense to have an easy way to fetch the format version from the `PositionDeletesTable`, since it basically translates 1:1 to the underlying table and so the format version of the `PositionDeletesTable` would be the same as the underlying table . The question is whether we add this logic to `TableUtil` or to the calling site(s) and would like to hear what others think here /cc @amogh-jahagirdar @aokolnychyi
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1859417454
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ if (table instanceof SerializableTable) {
+ return formatVersion(((SerializableTable) table).underlyingTable());
+ } else if (table instanceof PositionDeletesTable) {
Review Comment:
I feel it is a bit weird to make this exception for `PositionDeletesTable` in core, given that we add it exclusively because of how the maintenance is written in Spark. I'd either return the underlying table format version for all metadata tables (which is pretty questionable as they don't really have a format version) or unwrap the underlying table in Spark. For instance, we already have [a class check](https://github.com/apache/iceberg/blob/f3560871564c95a0e7b2bff3ca6ecb2e08726d01/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkTable.java#L289) in `SparkTable`. We can do similar in the maintenance procedure.
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1872731920
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /** Returns the format version of the given table */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // SerializableMetadataTable is a subclass of SerializableTable but does not support
+ // operations()
+ if (table instanceof HasTableOperations
Review Comment:
> I think we still have a problem that getting SerializableTable here would require loading the metadata location and sending a request to the storage
Yes this is a trade-off here because I was trying to avoid having effectively a mix of a Util class and adding `formatVersion()` to the `SerializableTable` API. I don't have a strong opinion here so I've done this in https://github.com/apache/iceberg/pull/11620/commits/1d8ef0554c8361dc8eadec66e9a68fb27bf58557 to show its implications
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852161094
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
Review Comment:
I agree with this approach. I just wonder if we should not define the format version just as table property (and set when using delete vectors for instance). It's more "hardcoded" but probably easier to evaluate (and fallback to V2 if the table property is not present).
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -120,7 +120,7 @@ private FileIO fileIO(Table table) {
return table.io();
}
- private Table lazyTable() {
+ protected Table lazyTable() {
Review Comment:
`protected` is for test purpose. Should we add `@VisibleForTesting` or similar (just as indication) ?
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852452253
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // being able to read the format version from the PositionDeletesTable is mainly needed in
+ // SparkPositionDeletesRewrite when determining whether to rewrite V2 position deletes to DVs
+ if (table instanceof BaseMetadataTable) {
Review Comment:
Shouldn't we just be looking at the underlying table then? Shouldn't the converter look at the base table rather than the metadata table?
Ie
`formatVersion(metadataTable.baseTable)`
Rather than
`formatVersion(metadataTable)`
Implicitly calling `metadataTable.baseTable` but only sometimes
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1859414335
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -143,6 +143,10 @@ protected Table newTable(TableOperations ops, String tableName) {
return new BaseTable(ops, tableName);
}
+ public Table underlyingTable() {
Review Comment:
Question: What about capturing the format version as a field in `SerializableTable`, similar to what we do for the metadata file location? The problem right now is that calling `lazyTable()` may actually require a request to load the metadata, which is something we would want to ideally avoid. Historically, we kept separate fields for what is considered important information and may be accessed frequently.
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -143,6 +143,10 @@ protected Table newTable(TableOperations ops, String tableName) {
return new BaseTable(ops, tableName);
}
+ public Table underlyingTable() {
Review Comment:
Question: What about capturing the format version as a field in `SerializableTable`, similar to what we do for the metadata file location? The problem right now is that calling `lazyTable()` may actually require a request to load the metadata, which is something we would want to ideally avoid. Historically, we kept separate fields for what is considered important information and might be accessed frequently.
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852565228
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // being able to read the format version from the PositionDeletesTable is mainly needed in
+ // SparkPositionDeletesRewrite when determining whether to rewrite V2 position deletes to DVs
+ if (table instanceof BaseMetadataTable) {
Review Comment:
that would require changing a bunch of more places, because effectively we have a `Broadcast<Table>` in `SparkPositionDeletesRewrite`: https://github.com/apache/iceberg/blob/8351248150c1750dc17b7b0156eeea8427423ccd/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeletesRewrite.java#L130-L131
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852570677
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ // being able to read the format version from the PositionDeletesTable is mainly needed in
+ // SparkPositionDeletesRewrite when determining whether to rewrite V2 position deletes to DVs
+ if (table instanceof BaseMetadataTable) {
Review Comment:
That is a an argument for ease of current implementation. But do you think it's the right decision going forward to have a format version for position deletes metadata table and none of the other metadata tables?
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1852471394
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /**
+ * Returns the format version of the given table. Note that subclasses of {@link
+ * BaseMetadataTable} do not have a format version. The only exception to this is {@link
+ * PositionDeletesTable}, where the format version is fetched from the underlying table.
+ */
+ public static int formatVersion(Table table) {
Review Comment:
OK, fair enough (good point about mutable/user facing aspect). I'm just a little "concerned" about the "complexity" of this method to get the format version (it remembers me the hack we did in JDBC 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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1859414335
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -143,6 +143,10 @@ protected Table newTable(TableOperations ops, String tableName) {
return new BaseTable(ops, tableName);
}
+ public Table underlyingTable() {
Review Comment:
Question: What about capturing the format version as a field in `SerializableTable`, similar to what we do for the metadata file location? The problem right now is that calling `lazyTable()` may actually require a request to load the metadata, which is something we would want to ideally avoid.
--
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
Re: [PR] Core: Add TableUtil to provide access to a table's format version [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1884367833
##########
core/src/main/java/org/apache/iceberg/TableUtil.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class TableUtil {
+ private TableUtil() {}
+
+ /** Returns the format version of the given table */
+ public static int formatVersion(Table table) {
+ Preconditions.checkArgument(null != table, "Invalid table: null");
+
+ if (table instanceof SerializableTable) {
+ SerializableTable serializableTable = (SerializableTable) table;
+ return serializableTable.formatVersion();
+ } else if (table instanceof HasTableOperations) {
+ return ((HasTableOperations) table).operations().current().formatVersion();
Review Comment:
Optional: Same comment about an extra variable for `TableOperations`.
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -158,6 +160,21 @@ public Map<String, String> properties() {
return properties;
}
+ public int formatVersion() {
+ return formatVersion;
+ }
+
+ private int formatVersion(Table table) {
+ if (table instanceof HasTableOperations) {
+ return ((HasTableOperations) table).operations().current().formatVersion();
+ } else {
+ // formatVersion=-1 will never be used/returned, because
Review Comment:
Optional: We may actually avoid the override by doing something like this below.
```
public int formatVersion() {
if (formatVersion == UNKNOWN_FORMAT_VERSION) {
throw new UnsupportedOperationException("Format version is unknown");
}
return formatVersion;
}
private int formatVersion(Table table) {
if (table instanceof HasTableOperations) {
...
} else {
return UNKNOWN_FORMAT_VERSION;
}
}
```
This would also cover cases for regular tables that didn't have `TableOperations` under the hood.
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -158,6 +160,21 @@ public Map<String, String> properties() {
return properties;
}
+ public int formatVersion() {
+ return formatVersion;
+ }
+
+ private int formatVersion(Table table) {
+ if (table instanceof HasTableOperations) {
+ return ((HasTableOperations) table).operations().current().formatVersion();
Review Comment:
Optional: What about an extra variable for `TableOperations`? We have an explicit casts + 3 method invocations, it may be easier to read with code on multiple lines and it would be consistent with the `metadataFileLocation` 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: 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