feat: Ensure data read order reflects commit sequence in Iceberg tables #6341

malhotrashivam · 2024-11-07T09:57:33Z

Before this change:
Data files were read in the order they appeared in the snapshot, which sometimes led to new data appearing at the top of the table instead of at the bottom.

This PR:

Fixes the ordering of data files to reflect the commit sequence, ensuring that newer data appears at the end of the table, as expected.
Also, allows comparison between different TableLocationKey implementations using fully qualified class names as a falback.

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java

lbooker42

This looks good.

…mber, pos)

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java

...ions/iceberg/src/main/java/io/deephaven/iceberg/location/IcebergTableParquetLocationKey.java

devinrsmith

One small thing left.

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java

...ions/iceberg/src/main/java/io/deephaven/iceberg/location/IcebergTableParquetLocationKey.java

rcaudy · 2024-11-15T18:00:22Z

...ions/iceberg/src/main/java/io/deephaven/iceberg/location/IcebergTableParquetLocationKey.java

+            return uri.compareTo(otherTyped.uri);
+        }
+        // When comparing with non-iceberg location key, we want to compare both partitions and URI
+        return super.compareTo(other);


This is a good first step, but I'm realizing we have some trouble here.
We may have neglected this, but TLKs really must be comparable to all TLKs, not just TLKs of the same provenance.
We need fallback comparisons.

We have two paths forward:

Keep behavior as-is, and defer to a new issue: Go back to throwing an exception on this line for now. Create an issue to define an ordering.

Fix the problem now.

My proposal for fixing, whether we do it here or in a new issue:

Make unrelated implementations compare to one another based on fully-qualified class name. This imposes an arbitrary but consistent ordering, and should roughly keep distinct sources together.

Make PartitionedTLK use order + partitions, followed by classname for tie-breaking.

Make URITLK order + partitions, then break ties by URI (if the other is a URITLK), and lastly break ties by classname.

Make IcebergTLK compare to other IcbergTLKs by Catalog name and TableIdentifier (which should also be added to equals and hashcode), then as currently defined in this PR, then to other URITLKs as defined by the URITLK rules, and so on.

Might want opinions from @abaranec and @devinrsmith

rcaudy

.

...-utils/src/main/java/io/deephaven/engine/testutil/locations/TableBackedTableLocationKey.java

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java

rcaudy · 2024-11-18T19:53:21Z

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java

+            return new IcebergTableParquetLocationKey(tableUuid, catalogName, tableIdentifier, manifestFile, dataFile,
+                    fileUri, 0, partitions, parquetInstructions, channelsProvider);


That sure is a lot of constructor args. Sorry. Builder? (Probably not worth it for something called in only once place.)

…able

rcaudy

I'm happy if @abaranec and @devinrsmith are.

...ions/iceberg/src/main/java/io/deephaven/iceberg/location/IcebergTableParquetLocationKey.java

extensions/iceberg/src/main/java/io/deephaven/iceberg/util/IcebergTableAdapter.java

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java

extensions/iceberg/src/main/java/io/deephaven/iceberg/util/IcebergUtils.java

...ions/iceberg/src/main/java/io/deephaven/iceberg/location/IcebergTableParquetLocationKey.java

rcaudy

.

Added ordering for discovery of manifest files in a snapshot

27a134c

malhotrashivam added feature request New feature or request NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed iceberg labels Nov 7, 2024

malhotrashivam added this to the 0.37.0 milestone Nov 7, 2024

malhotrashivam requested a review from lbooker42 November 7, 2024 09:57

malhotrashivam self-assigned this Nov 7, 2024

malhotrashivam commented Nov 7, 2024

View reviewed changes

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java Outdated Show resolved Hide resolved

Merge branch 'main' into nightly/sm-ice-read-order

5af1281

malhotrashivam requested a review from devinrsmith November 7, 2024 15:01

lbooker42 reviewed Nov 7, 2024

View reviewed changes

malhotrashivam added 3 commits November 8, 2024 00:08

Added ordering of Iceberg keys based on number of files discovered

4799ed5

Sorted data files on dataSequenceNumber while reading

9a72894

Added data file ordering based on (dataSequenceNumber, fileSequenceNu…

e9f2e66

…mber, pos)

malhotrashivam commented Nov 7, 2024

View reviewed changes

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java Outdated Show resolved Hide resolved

devinrsmith reviewed Nov 7, 2024

View reviewed changes

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java Outdated Show resolved Hide resolved

devinrsmith reviewed Nov 7, 2024

View reviewed changes

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java Outdated Show resolved Hide resolved

Review with Devin contd.

a0e209e

malhotrashivam requested a review from devinrsmith November 8, 2024 04:55

devinrsmith requested changes Nov 8, 2024

View reviewed changes

...ions/iceberg/src/main/java/io/deephaven/iceberg/location/IcebergTableParquetLocationKey.java Outdated Show resolved Hide resolved

Review with Devin Part 2

3c116cb

devinrsmith reviewed Nov 8, 2024

View reviewed changes

...ions/iceberg/src/main/java/io/deephaven/iceberg/location/IcebergTableParquetLocationKey.java Outdated Show resolved Hide resolved

Added manifest sequence number for ordering of data files

b048c64

devinrsmith reviewed Nov 11, 2024

View reviewed changes

extensions/iceberg/src/main/java/io/deephaven/iceberg/layout/IcebergBaseLayout.java Outdated Show resolved Hide resolved

Updated comments as per code review

c9a4e86

malhotrashivam requested a review from devinrsmith November 11, 2024 15:39

devinrsmith previously approved these changes Nov 11, 2024

View reviewed changes

rcaudy reviewed Nov 11, 2024

View reviewed changes

Review with Ryan part 1

f42b602

Review with Ryan contd.

9116c63

malhotrashivam dismissed devinrsmith’s stale review via 9116c63 November 15, 2024 08:26

rcaudy reviewed Nov 15, 2024

View reviewed changes

malhotrashivam added 3 commits November 16, 2024 00:04

Merge branch 'main' into nightly/sm-ice-read-order

ad2bf0e

Changed signrature of IcebergTableParquetLocationKey constructor

9a69898

Removed an extra newline

0be2ff8

rcaudy reviewed Nov 15, 2024

View reviewed changes

Review with Ryan contd.

ceac1b2

rcaudy reviewed Nov 18, 2024

View reviewed changes

Using toString instead of name for tableIdentifier and renamed a vari…

814b96f

…able

malhotrashivam requested review from abaranec and devinrsmith November 18, 2024 20:07

rcaudy previously approved these changes Nov 18, 2024

View reviewed changes

devinrsmith requested changes Nov 19, 2024

View reviewed changes

Updated PR based on review comments by Devin

e358605

malhotrashivam dismissed rcaudy’s stale review via e358605 November 19, 2024 14:34

malhotrashivam added 2 commits November 19, 2024 22:41

More changes to comparator logic

c994561

Some more changes

7af3345

devinrsmith reviewed Nov 19, 2024

View reviewed changes

More review comments

503af9f

rcaudy reviewed Nov 19, 2024

View reviewed changes

malhotrashivam added 2 commits November 20, 2024 00:10

Including patch from Ryan

e762fbc

Including patch from Ryan

22aee1d

rcaudy previously approved these changes Nov 19, 2024

View reviewed changes

Moved TableIdentifierComparator next to location key

c440d6c

malhotrashivam dismissed rcaudy’s stale review via c440d6c November 19, 2024 19:56

rcaudy approved these changes Nov 19, 2024

View reviewed changes

devinrsmith approved these changes Nov 19, 2024

View reviewed changes

malhotrashivam merged commit 6127a39 into deephaven:main Nov 20, 2024
18 checks passed

github-actions bot locked and limited conversation to collaborators Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Ensure data read order reflects commit sequence in Iceberg tables #6341

feat: Ensure data read order reflects commit sequence in Iceberg tables #6341

malhotrashivam commented Nov 7, 2024 •

edited

Loading

lbooker42 left a comment

devinrsmith left a comment

rcaudy Nov 15, 2024

rcaudy left a comment

rcaudy Nov 18, 2024

rcaudy left a comment

rcaudy left a comment

		return new IcebergTableParquetLocationKey(tableUuid, catalogName, tableIdentifier, manifestFile, dataFile,
		fileUri, 0, partitions, parquetInstructions, channelsProvider);

feat: Ensure data read order reflects commit sequence in Iceberg tables #6341

feat: Ensure data read order reflects commit sequence in Iceberg tables #6341

Conversation

malhotrashivam commented Nov 7, 2024 • edited Loading

lbooker42 left a comment

Choose a reason for hiding this comment

devinrsmith left a comment

Choose a reason for hiding this comment

rcaudy Nov 15, 2024

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

rcaudy Nov 18, 2024

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

malhotrashivam commented Nov 7, 2024 •

edited

Loading