-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support JDBC catalog in Iceberg connector #11772
Conversation
3409dd0
to
d2eb975
Compare
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/JdbcIcebergClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/JdbcIcebergClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/JdbcIcebergClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/JdbcIcebergClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/JdbcIcebergClient.java
Outdated
Show resolved
Hide resolved
...ino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/IcebergJdbcCatalogModule.java
Outdated
Show resolved
Hide resolved
...iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcIcebergConfig.java
Outdated
Show resolved
Hide resolved
Very excited to see this getting traction. Just wanted to make a note here that we may expect to see a similar issue with #6850 if not accounted for. The iceberg catalog on spark sql writes and preserves upper cases in schema and table names through its jdbc connection, and this could break on Trino iceberg-jdbc catalog if not accounted for. We've been testing out our own versions of jdbc catalog on our iceberg plugin, and we have come across this issue that we have yet to resolve. Query In database, upper case is preserved as "exampleTable1" Query We do not see the same issue with lower cased table names |
d2eb975
to
5a6cd1f
Compare
5a6cd1f
to
95ce171
Compare
Do we have / want to have some compatibility tests with Spark? |
Can you provide some details on how to configure s3 access with jdbc catalog? For hive catalog, we add below config in iceberg.properties file.
Wondering if similar config is required with jdbc as well or some alternative exists that can access |
My understanding is that it would be the same config items you've listed (and testing with the |
the same configs are working with jdbc catalog as well. thx. |
95ce171
to
6012aeb
Compare
7a6f67d
to
298e8c3
Compare
Added support for product tests with Spark. |
298e8c3
to
1386ad5
Compare
...presto-product-tests/conf/environment/singlenode-spark-iceberg-jdbc-catalog/create-table.sql
Outdated
Show resolved
Hide resolved
1386ad5
to
9c4cda8
Compare
Hi @ebyhr : here's the recently merged PR in Apache Iceberg that resolves the Jdbc catalog reserved keyword conflict on NAMESPACE_PROPERTY_KEY for reference |
9c4cda8
to
a14c771
Compare
@@ -0,0 +1,20 @@ | |||
/** | |||
Table definition in Iceberg repository: | |||
https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we make sure this file stays in sync with the referenced one?
or, maybe we can avoid having a copy here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the namespace_properties_table changes along with others we were waiting for just got released on 0.14.0 a few days ago.
Maybe we could now reference the static variable JdbcUtil.CREATE_NAMESPACE_PROPERTIES_TABLE here going forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the namespace_properties_table changes along with others
what are the changes?
does it mean we should test this catalog against Iceberg 13 and Iceberg 14 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They changed column names in apache/iceberg#5017. The PR doesn't contain logic to handle the old table definition, so I suppose they just broke the backward compatibility.
does it mean we should test this catalog against Iceberg 13 and Iceberg 14 as well?
I think so. (or clarify the supported version in documentation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They changed column names in apache/iceberg#5017. The PR doesn't contain logic to handle the old table definition, so I suppose they just broke the backward compatibility.
Concerning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to this issue on Iceberg repo no backwards compatible was broken because the previous version of this was never released? apache/iceberg#4952
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact I can confirm the Iceberg guys didn't release a breaking change, see the file in question at 0.13.2 https://github.com/apache/iceberg/blob/apache-iceberg-0.13.2/core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java where this NAMESPACE_PROPERTY_KEY
didn't exist, and now see same file at 0.14.0, where it does exist with the new value https://github.com/apache/iceberg/blob/apache-iceberg-0.14.0/core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java#L83.
So sounds like it was never released with the old value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nfcampos Your confirmation looks correct. Do you know if they have compatibility test in the repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I know of, no
a14c771
to
20f4e93
Compare
Just rebased on upstream to resolve logical conflicts. |
cc @mosabua @bitsondatadev for how to best provide those warnings in the docs. |
@ebyhr, let's discuss some of the documentation phrasing on Slack. |
94de1c3
to
8b536f3
Compare
Rebased on upstream to resolve conflicts. |
8b536f3
to
e203020
Compare
Addressed comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please give @alexjo2144 or @findinpath a chance to read too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we respecting the jdbc.strict-mode
table property?
Do we have follow up issues for Views and MVs?
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/IcebergJdbcClient.java
Outdated
Show resolved
Hide resolved
public Connection openConnection() | ||
throws SQLException | ||
{ | ||
Connection connection = DriverManager.getConnection(connectionUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have some connection pool here to draw from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would skip supporting connection pools in this PR. We can add it later when needed.
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java
Outdated
Show resolved
Hide resolved
e203020
to
5dd2a4a
Compare
We shouldn't create tables on non-existing namespaces regardless of the properties and it's tested by
No because there's no plan to support them at this time. |
(Rebased on upstream to resolve conflicts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a docs review here.
Few small changes to the docs but they look good!
f0f86fa
to
ab3ad9b
Compare
Greetings @findepi and @bitsondatadev thank you for your diligent work on JDBC other connectors. Is there anything you need from the community to get this one across the line? I am happy to connect you with Dremio PMs or others that can provide clarifications or support. I see a lot of value in Trino and would love to see the query-where-it-lives grow to this catalog. We are believers in data as code and want to help evolve that space through technologies like Trino and Nessie. Thank you. |
The JDBC catalog was included in the 406 release a few weeks ago, so I would call it across the line. https://trino.io/docs/current/release/release-406.html#iceberg-connector Let us know if you have a chance to try it out, feedback is always welcome. |
#11701 Support for Nessie Catalogs is what I am talking about. There is a March 29, 2022 pull request in the queue for this.
From: Alexander Jo ***@***.***>
Sent: Thursday, February 9, 2023 2:44 PM
To: trinodb/trino ***@***.***>
Cc: Brandon Jackson ***@***.***>; Comment ***@***.***>
Subject: Re: [trinodb/trino] Support JDBC catalog in Iceberg connector (PR #11772)
External Email: This message originated from outside Empower Pharmacy. Please exercise caution before opening attachments, clicking links, replying, or providing information to the sender.
Is there anything you need from the community to get this one across the line?
The JDBC catalog was included in the 406 release a few weeks ago, so I would call it across the line. https://trino.io/docs/current/release/release-406.html#iceberg-connector
Let us know if you have a chance to try it out, feedback is always welcome.
—
Reply to this email directly, view it on GitHub<#11772 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A2SNAU6KPC77BKN6WWHVJ5DWWVJH7ANCNFSM5SOYYGFA>.
You are receiving this because you commented.Message ID: ***@***.******@***.***>>
|
Excuse me, I would like to ask how is the progress of this issue? We connect to Iceberg through the Postgre JDBC catalog, the following is the properties configuration of the configuration catalog.
We use spark sql to write table named
In addition, when we write to Iceberg in the same way but the table named |
Description
Support JDBC catalog in Iceberg connector.
Fixes #9968
Release notes
(x) Release notes entries required with the following suggested text: