Core: Add TableUtil to provide access to a table's format version by nastra · Pull Request #11620 · apache/iceberg

nastra · 2024-11-21T13:48:46Z

This is an alternative impl to #11587

RussellSpitzer · 2024-11-21T15:19:05Z

+
+    // 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) {


I wish we had scala or even kotlin

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

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?

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

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

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

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(...)?

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.

that would require changing a bunch of more places, because effectively we have a Broadcast<Table> in SparkPositionDeletesRewrite:

iceberg/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeletesRewrite.java

Lines 130 to 131 in 8351248

Broadcast<Table> tableBroadcast =

sparkContext.broadcast(SerializableTableWithSize.copyOf(table));

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?

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

aokolnychyi · 2024-11-26T23:38:35Z

    return new BaseTable(ops, tableName);
  }

+  public Table underlyingTable() {


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.

aokolnychyi · 2024-12-05T22:58:31Z

+
+    // SerializableMetadataTable is a subclass of SerializableTable but does not support
+    // operations()
+    if (table instanceof HasTableOperations


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?

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 1d8ef05 to show its implications

nastra · 2024-12-06T07:45:23Z

  private transient volatile Schema lazySchema = null;
  private transient volatile Map<Integer, PartitionSpec> lazySpecs = null;
  private transient volatile SortOrder lazySortOrder = null;
-  private final UUID uuid;


moving this up to the other final fields

aokolnychyi

Looks good to me, some optional suggestions. Thanks for adding the tests too!

aokolnychyi · 2024-12-13T19:05:17Z

+      SerializableTable serializableTable = (SerializableTable) table;
+      return serializableTable.formatVersion();
+    } else if (table instanceof HasTableOperations) {
+      return ((HasTableOperations) table).operations().current().formatVersion();


Optional: Same comment about an extra variable for TableOperations.

aokolnychyi · 2024-12-13T19:07:23Z

+
+  private int formatVersion(Table table) {
+    if (table instanceof HasTableOperations) {
+      return ((HasTableOperations) table).operations().current().formatVersion();


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.

aokolnychyi · 2024-12-13T19:13:45Z

+    if (table instanceof HasTableOperations) {
+      return ((HasTableOperations) table).operations().current().formatVersion();
+    } else {
+      // formatVersion=-1 will never be used/returned, because


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.

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)

…pat parser Three of five reviewer-flagged items from the Anton/Ryan synthesis: 1. Salt out of the Catalyst resolution inner loop ApplyReadRestrictions previously generated a new SecureRandom salt inside the per-attribute match inside resolveOperators, so fixed-point re-entry could regenerate the salt mid-planning and break determinism within a query. Generate the salt once per apply() call and thread it through buildMaskExpression so all Sha256QueryLocal actions in the rewrite share a stable salt. 2. Rename ReadRestrictionsAware -> SupportsReadRestrictions Match the capability-marker naming used elsewhere in Iceberg (SupportsDistributedScanPlanning, SupportsReplaceView, etc.). Centralize the sniff in TableUtil.readRestrictions(Table) following the TableUtil.formatVersion precedent (apache#11620), so engines have one decision locus and future Flink/Trino integrations don't re-invent the check. 3. Forward-compat for unknown action discriminators ActionParser used to throw IllegalArgumentException on an unrecognized action type, blocking older clients from interoperating with newer servers. Introduce Action.Unknown that preserves (fieldId, rawTypeString) so parsing is lossless; enforcement (Actions.bind, rule binding) fails closed when it encounters Unknown so silent bypass of unmasked data is impossible. Mirrors ImmutableUnknownViewRepresentation in the view spec. The existing 'unknownActionTypeRejected' test is renamed and updated to assert preservation instead of rejection. Deferred to follow-up PRs: - Bind row filter at parse time (needs schema plumbing in LoadTableResponseParser) - TestApplyReadRestrictions + TestReadRestrictedQuery (needs SparkSession fixture)

github-actions Bot added the core label Nov 21, 2024

nastra mentioned this pull request Nov 21, 2024

API, Core: Add formatVersion() to Table #11587

Closed

jbonofre reviewed Nov 21, 2024

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/SerializableTable.java Outdated

Comment thread core/src/main/java/org/apache/iceberg/TableUtil.java

RussellSpitzer reviewed Nov 21, 2024

View reviewed changes

nastra force-pushed the table-util branch 3 times, most recently from 4515c45 to 8c69d34 Compare November 22, 2024 10:03

nastra requested a review from RussellSpitzer November 26, 2024 16:50

aokolnychyi reviewed Nov 26, 2024

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TableUtil.java Outdated

nastra force-pushed the table-util branch 2 times, most recently from bdfc988 to 13a016c Compare November 27, 2024 07:57

nastra requested review from amogh-jahagirdar and aokolnychyi November 27, 2024 08:00

aokolnychyi reviewed Dec 5, 2024

View reviewed changes

nastra force-pushed the table-util branch from 13a016c to 1d8ef05 Compare December 6, 2024 07:37

nastra commented Dec 6, 2024

View reviewed changes

nastra force-pushed the table-util branch from 2fbe20d to 5df9f8d Compare December 9, 2024 15:38

aokolnychyi approved these changes Dec 13, 2024

View reviewed changes

amogh-jahagirdar approved these changes Dec 15, 2024

View reviewed changes

nastra added 2 commits December 16, 2024 07:53

Core: Add TableUtil to provide access to a table's format version

3c3c339

Store formatVersion as field in SerializableTable

975d302

nastra force-pushed the table-util branch from 5df9f8d to e089db9 Compare December 16, 2024 07:48

updates

98ab07d

nastra force-pushed the table-util branch from e089db9 to 98ab07d Compare December 16, 2024 07:50

nastra merged commit f40ec20 into apache:main Dec 16, 2024

nastra deleted the table-util branch December 16, 2024 10:10

nastra mentioned this pull request Feb 27, 2025

Spark: Rewrite V2 deletes to V3 DVs #12250

Merged

	Broadcast<Table> tableBroadcast =
	sparkContext.broadcast(SerializableTableWithSize.copyOf(table));

Uh oh!

Conversation

nastra commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nastra commented Nov 21, 2024 •

edited

Loading

aokolnychyi Nov 26, 2024 •

edited

Loading

aokolnychyi Dec 13, 2024 •

edited

Loading

aokolnychyi Dec 13, 2024 •

edited

Loading