feat!: Updated API for Iceberg read operations #6268

malhotrashivam · 2024-10-22T23:07:34Z

BREAKING CHANGES / Documentation Update:

As a follow up to #5707, following are the main changes:

Rename IcebergInstructions to IcebergReadInstructions
Moved all arguments from read APIs for IcebergTableAdaper::definition, IcebergTableAdaper::definitionTable, and IcebergTableAdaper::table to inside IcebergReadInstructions to reduce the overloads.

For example,

Before

iceberg_instructions = IcebergInstructions.builder()
    .updateMode(IcebergUpdateMode.autoRefreshingMode(1_000L))
    .build()
sales_multi = tableAdapter.table(848129305390678414L, iceberg_instructions) 
// 848129305390678414L is the snapshot ID to be loaded

After

iceberg_instructions = IcebergInstructions.builder()
    .snapshotId(848129305390678414L)
    .updateMode(IcebergUpdateMode.autoRefreshingMode(1_000L))
    .build()
sales_multi = tableAdapter.table(iceberg_instructions)

extensions/iceberg/s3/src/test/java/io/deephaven/iceberg/util/IcebergToolsTest.java

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

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

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

lbooker42 · 2024-10-23T15:35:39Z

One more thing, the nesting of IcebergReadInstructions inside IcebergReadTable makes me wonder if we really need two levels at all? Can we specialize IcebergReadInstructions to include schema, snapshot, etc. and only have one builder? Not sure that makes sense in writing though

py/server/deephaven/experimental/iceberg.py

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

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

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

lbooker42 · 2024-10-23T17:34:24Z

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

-
-        // Load the table from the catalog.
-        return definition(tableSnapshot, instructions);
+    private static final class SpecAndSchema {


I might argue that Snapshot should be represented here rather than a re-written instruction object.

Actually, maybe not. Rewriting might be exactly what we want. @devinrsmith wants to include Schema instructions in these calls so maybe populating the instructions with snapshot now (and Schema later) is the right idea.

jmao-denver

The Python changes LGTM.
Do we need to tag it as 'breaking' even if it is experimental? Just for release notes purpose?

malhotrashivam · 2024-10-25T15:55:09Z

The Python changes LGTM. Do we need to tag it as 'breaking' even if it is experimental? Just for release notes purpose?

I tagged it breaking also from Java point of view because we are changing all the iceberg read APIs.

chipkent

Python LGTM

rcaudy

I like it. Minor questions/quibbles only.

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

devinrsmith

I think we need to follow up right after this with #6124, at least plumbing-wise.

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

deephaven-internal · 2024-10-28T21:54:37Z

Labels indicate documentation is required. Issues for documentation have been opened:

Community: deephaven/deephaven-docs-community#335

malhotrashivam added 2 commits October 22, 2024 18:02

Renamed IcebergInstructions to IcebergReadInstructions

dbd8ebd

Modified the read API to use immutables

394010e

malhotrashivam added documentation Improvements or additions to documentation breaking ReleaseNotesNeeded Release notes are needed iceberg labels Oct 22, 2024

malhotrashivam added this to the 0.37.0 milestone Oct 22, 2024

malhotrashivam requested review from lbooker42 and devinrsmith October 22, 2024 23:07

malhotrashivam self-assigned this Oct 22, 2024

malhotrashivam requested review from chipkent, jmao-denver and rcaudy as code owners October 22, 2024 23:07

malhotrashivam commented Oct 22, 2024

View reviewed changes

extensions/iceberg/s3/src/test/java/io/deephaven/iceberg/util/IcebergToolsTest.java Outdated Show resolved Hide resolved

malhotrashivam added DocumentationNeeded and removed documentation Improvements or additions to documentation labels Oct 22, 2024

lbooker42 reviewed Oct 23, 2024

View reviewed changes

Review with Larry Part 1

680daae

malhotrashivam commented Oct 23, 2024

View reviewed changes

py/server/deephaven/experimental/iceberg.py Outdated Show resolved Hide resolved

Minor tweaks

622749c

lbooker42 reviewed Oct 23, 2024

View reviewed changes

malhotrashivam added 3 commits October 23, 2024 11:55

Review with Larry Part 2

6534d5f

Review with Larry Part 3

69227f1

Minor rewording comments

acf81a0

lbooker42 reviewed Oct 23, 2024

View reviewed changes

Review with Larry Part 4

650d70e

lbooker42 previously approved these changes Oct 23, 2024

View reviewed changes

Added python support and updated tests

315ea18

malhotrashivam dismissed lbooker42’s stale review via 315ea18 October 23, 2024 19:38

malhotrashivam mentioned this pull request Oct 23, 2024

feat: Added support to write iceberg tables #5989

Merged

malhotrashivam added 2 commits October 23, 2024 15:14

Fix the failing test

e125813

Removing some extra includes

b254ceb

jmao-denver reviewed Oct 25, 2024

View reviewed changes

jmao-denver changed the title ~~feat: Updated API for Iceberg read operations~~ feat!: Updated API for Iceberg read operations Oct 25, 2024

chipkent previously approved these changes Oct 25, 2024

View reviewed changes

rcaudy reviewed Oct 25, 2024

View reviewed changes

extensions/iceberg/src/main/java/io/deephaven/iceberg/util/IcebergTableAdapter.java Outdated Show resolved Hide resolved

Review with Ryan

d2e1723

malhotrashivam dismissed chipkent’s stale review via d2e1723 October 25, 2024 21:23

rcaudy previously approved these changes Oct 25, 2024

View reviewed changes

devinrsmith reviewed Oct 25, 2024

View reviewed changes

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

Review with Devin Part 1

61d8a91

malhotrashivam dismissed rcaudy’s stale review via 61d8a91 October 28, 2024 16:40

malhotrashivam requested a review from devinrsmith October 28, 2024 18:16

devinrsmith approved these changes Oct 28, 2024

View reviewed changes

rcaudy approved these changes Oct 28, 2024

View reviewed changes

malhotrashivam merged commit 703f527 into deephaven:main Oct 28, 2024
17 checks passed

github-actions bot locked and limited conversation to collaborators Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Updated API for Iceberg read operations #6268

feat!: Updated API for Iceberg read operations #6268

malhotrashivam commented Oct 22, 2024 •

edited

Loading

lbooker42 commented Oct 23, 2024 •

edited

Loading

lbooker42 Oct 23, 2024

lbooker42 Oct 23, 2024

jmao-denver left a comment

malhotrashivam commented Oct 25, 2024

chipkent left a comment

rcaudy left a comment

devinrsmith left a comment

deephaven-internal commented Oct 28, 2024

feat!: Updated API for Iceberg read operations #6268

feat!: Updated API for Iceberg read operations #6268

Conversation

malhotrashivam commented Oct 22, 2024 • edited Loading

lbooker42 commented Oct 23, 2024 • edited Loading

lbooker42 Oct 23, 2024

Choose a reason for hiding this comment

lbooker42 Oct 23, 2024

Choose a reason for hiding this comment

jmao-denver left a comment

Choose a reason for hiding this comment

malhotrashivam commented Oct 25, 2024

chipkent left a comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

devinrsmith left a comment

Choose a reason for hiding this comment

deephaven-internal commented Oct 28, 2024

malhotrashivam commented Oct 22, 2024 •

edited

Loading

lbooker42 commented Oct 23, 2024 •

edited

Loading