BigQuery: Eliminate redundant table load by using ETag for conflict detection #14940

joyhaldar · 2025-12-28T07:19:55Z

The current commit path loads the BigQuery table twice:

During table refresh to get metadata location
During commit to get ETag for the update call

This change stores the table from the refresh step and reuses it during commit, eliminating the redundant load. Concurrent modification detection remains intact via ETag-based optimistic locking in the BigQuery API.

BigQuery API calls per commit:

Before	After
`doRefresh` → loads table	`doRefresh` → loads table
`updateTable` → loads table again	reuses table from refresh

This improves commit latency and reduces tables.get quota consumption.

Changes:

Store table loaded during refresh as metastoreTable for reuse during commit
Use Preconditions.checkState() to ensure table is loaded before commit
Remove metadata location comparison which is redundant with ETag check
Update test to verify ETag-based conflict detection

… calls Cache the Table object loaded in doRefresh() for reuse in updateTable(), eliminating a redundant tables.get call per commit. Concurrent modification detection is preserved via ETag based optimistic locking in tables.patch.

manuzhang · 2025-12-30T10:20:01Z

bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryTableOperations.java

    try {
-      metadataLocation =
-          loadMetadataLocationOrThrow(client.load(tableReference).getExternalCatalogTableOptions());
+      Table table = client.load(tableReference);


why do we need this local variable?

why do we need this local variable?

Thank you for your review Manu. I used the local variable for readability, but happy to inline if you think it's a good idea.

Agree that it's unnecessary.

manuzhang · 2025-12-30T10:24:48Z

bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryTableOperations.java

    ExternalCatalogTableOptions options = table.getExternalCatalogTableOptions();
    addConnectionIfProvided(table, metadata.properties());

-    // If `metadataLocationFromMetastore` is different from metadata location of base, it means


why is this check removed?

why is this check removed?

Thank you for your review Manu.

This check becomes redundant with caching.

Before:

doRefresh() loads table -> metadata location = "v1"

Someone else commits -> metadata location = "v2"

updateTable() loads table again -> sees "v2"

Check catches: "v1" != "v2" -> fail

With caching:

doRefresh() loads table -> metadata location = "v1", cached

Someone else commits -> metadata location = "v2"

updateTable() uses cached table -> still sees "v1"

Check passes: "v1" == "v1" (compares against itself)

tables.patch fails with HTTP 412 (ETag mismatch) -> Iceberg retries

The ETag check in tables.patch catches the same conflict, so this check no longer adds value.

manuzhang · 2025-12-31T15:07:20Z

bigquery/src/test/java/org/apache/iceberg/gcp/bigquery/TestBigQueryTableOperations.java


  @Test
-  public void failWhenMetadataLocationDiff() throws Exception {
+  public void failWhenConcurrentModificationDetected() throws Exception {


do you verify table is only loaded once?

Thank you for the review Manu. Sorry about that, I have added verification to confirm table is loaded only once in this commit.

Verify table is loaded only once in test

joyhaldar · 2026-01-07T17:22:29Z

Hello @talatuyarer, @rambleraptor, could I please request you to review this PR when you have some time?

rambleraptor

My concerns are mostly around making sure that concurrent changes are respected. I agree we can use the ETag for this purpose, so this sounds good to me! Thanks for writing this

kevinjqliu

i dont think we should couple table refresh with table update. Maybe i'm missing nuisance of this implementation. @talatuyarer wdyt?

kevinjqliu · 2026-01-08T17:56:45Z

bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryTableOperations.java

+    Table table = this.refreshedTable;
+    if (table == null) {
+      LOG.warn("Table not set from doRefresh() for {}, loading from BigQuery", tableName());
+      table = client.load(tableReference);
+    }
+
+    this.refreshedTable = null;


this feels like an anti-pattern to me, and should not live in updateTable. i think we should separate concerns for table refresh and update.
Looking at JdbcTableOperations, updateTable should just be an atomic operation.

I would agree with @kevinjqliu's comment here. I think it's safe to assume that the member reference is not null (or we can add a check, but I don't see the scenario where we would reload). We also don't need the local variable in this case.

Addressed, removed the defensive reload and replaced with Preconditions.checkState(). Let me know if you'd like to skip the check entirely.

danielcweeks

A few comments, but I think we either want to just track the metastoreTable explicitly or just use the ETag which appears to be equivalent in the current usage.

danielcweeks · 2026-01-08T21:35:41Z

bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryTableOperations.java

  private final TableReference tableReference;

+  /** Table loaded in doRefresh() for reuse in updateTable() to avoid redundant API call. */
+  private volatile Table refreshedTable;


Do we need the full table here? It looks like what we're doing is replacing the location check with an ETag check, which is fine, but then we just need the ETag, correct?

Also, if we want to preserve the table, I think we should change the name to metastoreTable since it technically just refers to the metastore's representation of the table and even though the method is called refresh, it's also used just for the initial load.

Done, renamed to metastoreTable.

We need the full Table in my understanding because updateTable() calls getExternalCatalogTableOptions(), addConnectionIfProvided(), and passes it to client.update().

danielcweeks · 2026-01-08T21:41:38Z

bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryTableOperations.java

    try {
-      metadataLocation =
-          loadMetadataLocationOrThrow(client.load(tableReference).getExternalCatalogTableOptions());
+      Table table = client.load(tableReference);


Agree that it's unnecessary.

danielcweeks · 2026-01-08T21:49:26Z

bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryTableOperations.java

-      String oldMetadataLocation, String newMetadataLocation, TableMetadata metadata) {
-    Table table = client.load(tableReference);
+  private void updateTable(String newMetadataLocation, TableMetadata metadata) {
+    Table table = this.refreshedTable;


nit: we only use the this. member reference on assignment

- Rename refreshedTable to metastoreTable - Remove unnecessary local variables in doRefresh() and updateTable() - Replace defensive reload with Preconditions.checkState() - Use this. prefix only on assignment

danielcweeks

Thanks @joyhaldar !

github-actions bot added the GCP label Dec 28, 2025

manuzhang reviewed Dec 30, 2025

View reviewed changes

manuzhang reviewed Dec 31, 2025

View reviewed changes

Verify table is loaded only once in test

f5cf2ec

Verify table is loaded only once in test

rambleraptor approved these changes Jan 7, 2026

View reviewed changes

kevinjqliu reviewed Jan 8, 2026

View reviewed changes

danielcweeks reviewed Jan 8, 2026

View reviewed changes

BigQuery: Address review feedback for ETag conflict detection

a0e358e

- Rename refreshedTable to metastoreTable - Remove unnecessary local variables in doRefresh() and updateTable() - Replace defensive reload with Preconditions.checkState() - Use this. prefix only on assignment

joyhaldar changed the title ~~BigQuery: Reuse table from refresh during commit to reduce API calls~~ BigQuery: Eliminate redundant table load by using ETag for conflict detection Jan 9, 2026

joyhaldar added 2 commits January 9, 2026 12:14

Remove stray blank line

2ce022f

Remove stray blank line in updateTable

dd85b96

danielcweeks approved these changes Jan 15, 2026

View reviewed changes

danielcweeks merged commit 38cc881 into apache:main Jan 15, 2026
46 checks passed

BigQuery: Eliminate redundant table load by using ETag for conflict detection #14940

BigQuery: Eliminate redundant table load by using ETag for conflict detection #14940

Uh oh!

Conversation

joyhaldar commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyhaldar Dec 31, 2025 • 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!

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!

joyhaldar commented Jan 7, 2026

Uh oh!

rambleraptor left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu left a comment

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!

danielcweeks left a comment

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!

danielcweeks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joyhaldar commented Dec 28, 2025 •

edited

Loading

joyhaldar Dec 31, 2025 •

edited

Loading