feat!: Updated API for Iceberg read operations

malhotrashivam commented 1 month ago

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)

lbooker42 commented 4 weeks ago

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

malhotrashivam commented 3 weeks ago

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.

deephaven-internal commented 3 weeks ago

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

Community: https://github.com/deephaven/deephaven-docs-community/issues/335

deephaven / deephaven-core

feat!: Updated API for Iceberg read operations #6268