-
Notifications
You must be signed in to change notification settings - Fork 129
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
Bump iceberg to 0.14 #4738
Bump iceberg to 0.14 #4738
Conversation
…` has been removed
…ssieClient for Trino Consumption (projectnessie#4491)'
Iceberg to 0.14.0 client-nessie-version to 0.30.0 (as from Iceberg 0.14.0)
Codecov Report
@@ Coverage Diff @@
## main #4738 +/- ##
============================================
- Coverage 85.63% 84.60% -1.04%
+ Complexity 3604 3533 -71
============================================
Files 415 415
Lines 16974 16974
Branches 1453 1453
============================================
- Hits 14536 14361 -175
- Misses 1939 2120 +181
+ Partials 499 493 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
LGTM overall, one minor comment
// TODO only used in a single test | ||
// tableCatalog.refresh(); |
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 sure I understand this 🤔
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.
Ah - that refresh()
functionality is gone now (on NessieCatalog
since apache/iceberg#4491).
Updated the comment, but leaving it as a todo as a reminder.
/cc @nastra
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.
ah yeah the refresh()
shouldn't be necessary anymore. There was an issue back then that required calling this explicitly but I fixed this with the refactoring to the NessieClient
wrapper in Iceberg
@@ -82,7 +82,8 @@ public void initialize(String inputName, Map<String, String> options) { | |||
x -> x.replace(NessieUtil.NESSIE_CONFIG_PREFIX, ""); | |||
|
|||
this.api = | |||
createNessieClientBuilder(options.get(NessieUtil.CONFIG_CLIENT_BUILDER_IMPL)) | |||
createNessieClientBuilder( | |||
options.get(NessieUtil.NESSIE_CONFIG_PREFIX + "client-builder-impl")) |
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.
nit: Should we use the existing NessieConfigConstants.CONF_NESSIE_CLIENT_BUILDER_IMPL
?
@@ -69,8 +73,10 @@ dependencies { | |||
testImplementation(project(":nessie-spark-extensions-base")) { testJarCapability() } | |||
testImplementation(project(":nessie-spark-extensions")) |
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.
nit: Is it good idea to rename nessie-spark-extensions
to nessie-spark-3.1-extensions
(just to have a similar syntax as spark-3.2 module)? Now reader has to figure out which version of spark is nessie-spark-extensions
@@ -104,11 +105,15 @@ public void setConf(Configuration conf) { | |||
} | |||
|
|||
public void refresh() throws NessieNotFoundException { | |||
tableCatalog.refresh(); | |||
// TODO This NessieExtCatalog is a *hack* to get some Iceberg view support into Nessie. |
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.
the whole method can be just removed
No description provided.