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

Disallow querying delta lake properties table #10447

Merged

Conversation

findinpath
Copy link
Contributor

Extracted relevant commits for disallowing the querying of the $properties table on the delta lake tables in hive from the PR #10441

@@ -60,6 +63,9 @@
if (table.isEmpty()) {
throw new TableNotFoundException(tableName);
}
if (isDeltaLakeTable(table.get())) {
throw new TrinoException(HIVE_UNSUPPORTED_FORMAT, "Cannot query Delta Lake table");
Copy link
Member

Choose a reason for hiding this comment

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

nit: would be good to mention table name in error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified the exception message to its original format to be in sync with the other places where the exception is being thrown.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @phd3 we should provide table name in the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll make a separate commit then to update everywhere the exception message.

Copy link
Member

Choose a reason for hiding this comment

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

agreed, the io.trino.plugin.hive.HiveMetadata#getTableHandle can (and should) be updated in a separate commit.

@findinpath findinpath force-pushed the disallow-querying-delta-lake-properties-table branch 2 times, most recently from f93877c to 0b757ba Compare January 4, 2022 05:58
@findinpath findinpath requested a review from phd3 January 4, 2022 05:59
@findinpath findinpath force-pushed the disallow-querying-delta-lake-properties-table branch 2 times, most recently from 349c5a0 to 55da414 Compare January 4, 2022 15:43
@@ -60,6 +63,9 @@
if (table.isEmpty()) {
throw new TableNotFoundException(tableName);
}
if (isDeltaLakeTable(table.get())) {
throw new TrinoException(HIVE_UNSUPPORTED_FORMAT, "Cannot query Delta Lake table");
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @phd3 we should provide table name in the message.

@findinpath findinpath force-pushed the disallow-querying-delta-lake-properties-table branch from 55da414 to 9c252f8 Compare January 5, 2022 12:57
@findinpath findinpath force-pushed the disallow-querying-delta-lake-properties-table branch from 9c252f8 to acb1779 Compare January 5, 2022 12:58
@phd3 phd3 merged commit 209de67 into trinodb:master Jan 6, 2022
@phd3
Copy link
Member

phd3 commented Jan 6, 2022

Merged, thanks!

@github-actions github-actions bot added this to the 368 milestone Jan 6, 2022
@phd3 phd3 mentioned this pull request Jan 6, 2022
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.

3 participants