Acehaidrey commented 5 days ago

Fix changelog table bug for start time older than current snapshot

Problem

After upgrading to Iceberg 1.5 and Spark 3.5.1, the create_changelog_view procedure occasionally returns records that were inserted outside (before) the specified time range. Specifically:

When querying for recent changes (e.g. last 10 minutes)
The view sometimes includes records from 3-5 hours ago
Re-running the same query returns the expected 0 records
This inconsistency wasn't present in Iceberg 1.3

Changes

Added a unit test testChangelogViewOutsideTimeRange to TestChangelogTable that:

Inserts test records into the table
Waits for a time period to ensure those records are "old"
Creates a changelog view with a time range that starts after the inserts
Verifies the view returns 0 records (expected behavior)
The test helps validate that records outside the specified time window are not incorrectly included

Testing

Added unit test directly targeting the time range filtering behavior
Test runs as part of the existing TestChangelogTable suite
Follows existing test patterns and conventions

To invoke the unit test: ./gradlew :iceberg-spark:iceberg-spark-extensions-3.5_2.12:test --tests "org.apache.iceberg.spark.extensions.TestChangelogTable"

Related Issue

TBD

Additional Context

This test was added to help investigate reported production issues where changelog views were inconsistently including historical records
The test provides a reproducible way to verify time range filtering behavior
@flyrain discussed this on this thread in slack: https://apache-iceberg.slack.com/archives/C025PH0G1D4/p1728070484459789

flyrain commented 2 days ago

Thank you, @Acehaidrey, for reporting this issue! It seems that the method buildChangelogScan() does not properly set up the scan when the startTimestamp is newer than the timestamp of the current snapshot. Adding the following logic at this line should address the problem:

if (startTimestamp != null && table.currentSnapshot().timestampMillis() < startTimestamp) {
  emptyScan = true;
}

However, I believe there's an opportunity to revisit the buildChangelogScan() method more holistically. Refactoring it could improve its clarity and make it less prone to errors in the future. I'd be happy to collaborate or discuss further ideas for restructuring the method. Let me know what you think!

flyrain commented 2 days ago

Hi @bryanck, do we still have time to include this bug fix in 1.7.1?

bryanck commented 1 day ago

The 1.7.1 ship has sailed, but if this is a regression or high priority, we can try to get it in.

flyrain commented 1 day ago

It is a regression, better to get it in. @Acehaidrey should be able to provide a quick fix soon, like today. Appreciate if you can hold 1.7.1 a bit more, @bryanck.

RussellSpitzer commented 1 day ago

@Acehaidrey

Execution failed for task ':iceberg-spark:iceberg-spark-extensions-3.5_2.12:spotlessJavaCheck'.
> The following files had format violations:
      src/test/java/org/apache/iceberg/spark/extensions/TestChangelogTable.java
          @@ -460,7 +460,6 @@
           ····//········[4,·"d",·"INSERT",·3,·7807948732874377651L]]

           ····assertThat(results).as("Num·records·must·be·zero").isEmpty();

          -
           ····//·Clean·up·the·changelog·view
           ····sql("DROP·VIEW·IF·EXISTS·test_changelog_view");
           ··}

RussellSpitzer commented 1 day ago

@flyrain + @Acehaidrey We expect this test to fail right? Like we need to still add a fix to this pr?