Skip to content
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

Fix reading Delta Table metadata from Glue #12164

Merged
merged 2 commits into from
Apr 28, 2022
Merged

Fix reading Delta Table metadata from Glue #12164

merged 2 commits into from
Apr 28, 2022

Conversation

erichwang
Copy link
Contributor

@erichwang erichwang commented Apr 27, 2022

Fixes: #12147

Caused by this PR:
#12130

The location field can sometimes be null (e.g. Iceberg tables, and
sometimes Delta Lake tables).
Delta Lake connector expects that the metastore storage descriptor be
fully translated if it exists. It only needs to return a dummy storage
value if no storage descriptor has been configured.
findepi
findepi previously approved these changes Apr 28, 2022
@findepi findepi changed the title Fix failing glue tests Fix reading Delta Table metadata from Glue Apr 28, 2022
@findepi
Copy link
Member

findepi commented Apr 28, 2022

Test PR: #12172
(cc @ilfrin @nineinchnick for potentially streamlining this)

@mosabua
Copy link
Member

mosabua commented Apr 28, 2022

@martint @electrum @dain @alexjo2144 .. this fixes the current release blocker... if we get this merged @martint and myself can get the release out today potentially.

// Default getter requires location to be set. Location is not set in the following scenarios:
// 1. CREATE TABLE flow in Hive connector sets delegate-transactional-managed-table-location-to-metastore to true and location is determined by HMS.
// 2. Iceberg format tables
// 3. Delta format tables (sometimes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked this today. Looks like we always expect the path to be in the storage descriptor. We could change that for Managed tables, but right now it's required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexjo2144, do you want me to remove the assumption that storage won't be specified for delta tables then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is fine, I made an Issue for follow up #12180

@findepi
Copy link
Member

findepi commented Apr 28, 2022

CI #12151

@erichwang
Copy link
Contributor Author

@findepi, it looks like the CI build with secrets failed on something that is transient. Can you kick it again and see if it passes?

@erichwang
Copy link
Contributor Author

erichwang commented Apr 28, 2022

Does anyone else have any concerns here? Otherwise, I think this is good to go.

@dain dain merged commit 49381ba into trinodb:master Apr 28, 2022
@github-actions github-actions bot added this to the 379 milestone Apr 28, 2022
v-jizhang added a commit to v-jizhang/presto that referenced this pull request May 17, 2022
Cherry-pick of trinodb/trino#12164

Delta Lake connector expects that the metastore storage descriptor be
fully translated if it exists. It only needs to return a dummy storage
value if no storage descriptor has been configured.

Co-authored-by: Eric Hwang <eric.hwang@starburstdata.com>
beinan pushed a commit to prestodb/presto that referenced this pull request May 18, 2022
Cherry-pick of trinodb/trino#12164

Delta Lake connector expects that the metastore storage descriptor be
fully translated if it exists. It only needs to return a dummy storage
value if no storage descriptor has been configured.

Co-authored-by: Eric Hwang <eric.hwang@starburstdata.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Glue tests are failing on master
5 participants