Support Iceberg refs system table

[jackye1995]

Description

Support refs system table in Iceberg

Additional context and related issues

Users can query the system table with:

SELECT * FROM "table$refs"

The schema is the same as the one in Iceberg repo: https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/RefsTable.java

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

[alexjo2144]

How do you feel about calling the table REFERENCES spelled out?

This also needs to be added to iceberg.rst for the docs here: https://trino.io/docs/current/connector/iceberg.html#metadata-tables

[jackye1995]

How do you feel about calling the table REFERENCES spelled out?

I am using REFS because that is what Iceberg core library uses, which means Spark and Flink system tables are also names as that. But if you think REFERENCES makes more sense in Trino, happy to name it that way.

[findinpath]

This also needs to be added to iceberg.rst for the docs here: https://trino.io/docs/current/connector/iceberg.html#metadata-tables

See for reference: https://github.com/apache/iceberg/blob/master/format/spec.md#snapshot-reference

[jackye1995]

@alexjo2144 updated everything except the name REFS, let me know if I need to name it to REFERENCES

[findinpath]

You could add in PageListBuilder new methods:

• appendInteger(Integer)
• appendBigint(Long)

to encapsulate the null check and make the code in this class more concise.

[jackye1995]

Added new methods as suggested

[alexjo2144]

How do you feel about calling the table REFERENCES spelled out?

I am using REFS because that is what Iceberg core library uses, which means Spark and Flink system tables are also names as that. But if you think REFERENCES makes more sense in Trino, happy to name it that way.

@ebyhr or @findepi either of you have thoughts on this? Matching the other engines vs spelling the whole word out.

[alexjo2144]

Looks good to me, pending input from maintainers on the table name.

[alexjo2144]

I think these are supposed to match width. @mosabua might want to take a look at the docs addition, but thanks for adding it

[jackye1995]

Updated to match the width

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

[bitsondatadev]

@jackye1995, would you mind replying to the last comments here please?

@ebyhr %comments, is this ready to be merged?

[jackye1995]

@ebyhr sorry did not notice you have one more comment, updated. Let me know if there is anything else needed.

[bitsondatadev]

@ebyhr sorry did not notice you have one more comment, updated. Let me know if there is anything else needed.

Thanks @jackye1995! I think there are a few other small naming fixes in #15646 if you can address those as well please! nvm you fixed it already!

After that @ebyhr should take a look shortly! You rock!

[electrum]

I think it's fine to match the other implementations, especially since the intent here is to have the same schema.

[jackye1995]

@electrum @ebyhr @findepi is there any plan to merge this and the related PRs for Iceberg reference support? We are about to cut Iceberg 1.2 branch, and have achieved all the same supports in Spark and Flink, would be great to have feature parity in Trino.

[ebyhr]

Could you rebase on master to resolve conflicts?

[jackye1995]

Can I get a confirmation if this PR is ever gonna be merged once rebased? If not I will just go ahead to apply the patch internally and you can close this PR instead of me continuing to rebase.

[bitsondatadev]

@jackye1995 I'm meeting with @electrum tomorrow I'll ask him to respond. I really want this one to land!

[bitsondatadev]

@jackye1995 would you also be able to do this change suggested by David to go back to REFS instead of REFERENCES?

https://trinodb.slack.com/archives/CP1MUNEUX/p1678319193961979?thread_ts=1677280433.219989&channel=CP1MUNEUX&message_ts=1678319193.961979

I will also update you on what David says about how close this is to merge.

[ebyhr]

would you also be able to do this change suggested by David to go back to REFS instead of REFERENCES?

The current implementation is already refs.

[bitsondatadev]

would you also be able to do this change suggested by David to go back to REFS instead of REFERENCES?

The current implementation is already refs.

Ooops 🫣🫠

[jackye1995]

Close PR, this will be available in AWS Athena by 3/31 in all AWS regions.

[electrum]

Sorry for the confusion on this. I'll merge after the test passes. Thanks for your work on this!

[bitsondatadev]

@electrum looks like the tests passed! Are we ready to merge this?

[electrum]

There was another conflict. Rebased again.

[findepi]

Per @ebyhr 's #16576 it seems the PR broke master builds.
@jackye1995 can you please help us and remind to /test-with-secrets?