Add Apache Iceberg load and export support#236
Conversation
Add ExportIcebergConfig to the Export oneof in transactions.proto with fields: catalog_uri, namespace, table_name, catalog_properties (warehouse, token, credential), schema, prefix, target_file_size_bytes, and compression. Grammar rules, parsers, printers, and tests are included for all three SDKs (Python, Go, Julia). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… into mjs-add-iceberg-export
…ferent mechanisms there but also keep it separate from general properties for privacy/security reasons
vustef
changed the title
There was a problem hiding this comment.
@reviewers, I'm not sure if this file is supposed to be generated...
There was a problem hiding this comment.
It's not (yet). We do want to extend ProtoBuf.jl to generate hash and equality as well, but this is not supported yet.
There was a problem hiding this comment.
@vustef could you also ask Claude to add tests for the new structs to https://github.com/RelationalAI/logical-query-protocol/blob/main/sdks/julia/LogicalQueryProtocol.jl/test/equality_tests.jl?
| # or appear in hashed structures (e.g. Dict keys, caches). | ||
| Base.:(==)(a::IcebergConfig, b::IcebergConfig) = | ||
| a.catalog_uri == b.catalog_uri && a.scope == b.scope && a.properties == b.properties | ||
| Base.hash(a::IcebergConfig, h::UInt) = hash(a.properties, hash(a.scope, hash(a.catalog_uri, h))) |
There was a problem hiding this comment.
@reviewers: I omitted auth_properties from hash and ==, thinking they shouldn't define object's identity. But this then influences that tests don't take this into account either. Not sure if this is fine, nor required.
… grammar.y itself
mjschleich
left a comment
mjschleich
left a comment
There was a problem hiding this comment.
Generally this looks good to me, just noticed two small issues
sdks/julia/LogicalQueryProtocol.jl/src/gen/relationalai/lqp/v1/transactions_pb.jl
Show resolved
Hide resolved
| RelationId table_def = 3; | ||
| repeated ExportIcebergColumn columns = 4; | ||
| optional string prefix = 5; |
There was a problem hiding this comment.
table_def and prefix may need a comment explaining what they are for
There was a problem hiding this comment.
It's just the default file name prefix used for parquet files during exports. Edit: maybe rename it to file_prefix.
…dtrip tests pass but only because we use *** as values in the tests (meaning we don't necesarily see whether we obfuscate values of auth_properties or not)
…gets passed to different pretty functions.
4 participants
Summary
ExportIcebergConfigmessage totransactions.protowith fields:locator,config(warehouse/token/credential),schema,prefix,target_file_size_bytes, andcompressionIcebergDatanode, reusingGNFColumns fromCSVData.Exportoneof to support Iceberg alongside CSVTest plan
make protobuf— regenerated bindings for Python, Go, Juliamake parsers/make printers— regenerated from grammarmake update-bins/make update-snapshots— test fixtures updatedmake test— all tests pass (Python 686, Julia 31509, Go ok)