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 hive.hive-views.run-as-invoker handling with legacy view translation #14077
Fix hive.hive-views.run-as-invoker handling with legacy view translation #14077
Conversation
...-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/env/common/Hadoop.java
Show resolved
Hide resolved
...resources/docker/presto-product-tests/common/hadoop/hive_with_run_view_as_invoker.properties
Outdated
Show resolved
Hide resolved
...resources/docker/presto-product-tests/common/hadoop/hive_with_run_view_as_invoker.properties
Show resolved
Hide resolved
@@ -150,6 +150,26 @@ public void testCurrentUser() | |||
.hasMessageContaining("Failed parsing stored view 'hive.default.current_user_hive_view': line 1:20: mismatched input '('"); | |||
} | |||
|
|||
@Test(groups = HIVE_VIEWS) | |||
public void testRunAsInvoker() |
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 this go into AbstractTestHiveViews
?
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.
If I understand correctly, it makes sense only if legacy hive views are used.
I am not an expert here though so I may be missing something.
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.
Is this relevant?
trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/ViewReaderUtil.java
Lines 88 to 95 in 6ceec25
if (isHiveViewsLegacyTranslation(session)) { | |
return new LegacyHiveViewReader(runHiveViewRunAsInvoker); | |
} | |
return new HiveViewReader( | |
new CoralSemiTransactionalHiveMSCAdapter(metastore, coralTableRedirectionResolver(session, tableRedirectionResolver, metadataProvider)), | |
typeManager, | |
runHiveViewRunAsInvoker); |
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 added a siimlar test in TestHiveViews
, since the one from legacy would not work.
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.
Why wouldn't it? what's the difference?
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.
Views created in hive won't work. We need to create one using Trino.
BTW your comment reaction time is absurdly good.
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 appears that you are correct. The error I got was something completely unrelated.
Moved the test to AbstractTestHiveViews
f87aa99
to
de3fd81
Compare
de3fd81
to
17ec1a5
Compare
testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveViews.java
Outdated
Show resolved
Hide resolved
@@ -49,7 +49,7 @@ public ConnectorViewDefinition decodeViewData(String viewData, Table table, Cata | |||
.map(column -> new ConnectorViewDefinition.ViewColumn(column.getName(), TypeId.of(column.getType().getTypeSignature().toString()))) | |||
.collect(toImmutableList()), | |||
Optional.ofNullable(table.getParameters().get(TABLE_COMMENT)), | |||
table.getOwner(), | |||
hiveViewsRunAsInvoker ? Optional.empty() : table.getOwner(), |
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 think similar should be done in the non-legacy reader https://github.com/trinodb/trino/pull/12221/files#r970434417
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 previous behavior with hiveViewsRunAsInvoker==false is the same as it was before, and with true the owner should not be passed on. So I believe there is nothing to change 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.
The PR changes LegacyHiveViewReader
and this looks OK.
Doesn't it need to change io.trino.plugin.hive.ViewReaderUtil.HiveViewReader#decodeViewData
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.
The non-legacy reader does already handle setting the owner correctly. HiveViewReader#decodeViewData
initially returns with an empty owner, but sets an owner, if using DEFINER and if the owner exists, in HiveMetadata#getView.
However, I think we can also just streamline this in HiveViewReader#decodeViewData
and not need the additional logic HiveMetadata#getView
.
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 like we smeared the responsibility across 3 places:
- legacy HiveViewReader
- non-legacy HiveViewReader
- HiveMetadata#getView
and the result is quite inconsistent (hence questions like above)
Looking into this more, i no longer understand why the change here is desireable.
We have the table.getOwner()
and it's some information, why do we need the LegacyHiveViewReader not to pass it further?
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.
OK, I get it.
But why would we pass hiveViewsRunAsInvoker ? Optional.empty() : table.getOwner()
here and
Optional.empty()
in non-legacy HiveViewReader?
HiveViewReader and LegacyHiveViewReader should differ in how they translate view SQL (with coral or not), but they should not differ in how they translate authorization/ownership, or whether the produce INVOKER or DEFINER view.
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.
That's part of the smeared responsibility that you mentioned. The non-legacy HiveViewReader passes an Optional.empty()
, but afterwards HiveMetadata#getView
will create another ConnectorViewDefinition
object with the proper owner set, if needed. I feel the same, which is why I added the extra commit.
If there is agreement with this, perhaps we can add that commit to this PR 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.
For now, let's have
hiveViewsRunAsInvoker ? Optional.empty() : table.getOwner(), | |
Optional.empty(), // will be filled in later by HiveMetadata |
and same in the other class.
Would that work?
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.
No objections from me. I can confirm that the suggested change will also resolve the issue. What do you think @skrzypo987?
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.
That would definitely be more consistent. Pushed the new version already
testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveViewsLegacy.java
Outdated
Show resolved
Hide resolved
17ec1a5
to
2fbca89
Compare
Looks like CI failed for some transient reason |
2fbca89
to
7b710c2
Compare
plugin/trino-hive/src/main/java/io/trino/plugin/hive/LegacyHiveViewReader.java
Show resolved
Hide resolved
7b710c2
to
4e74ebc
Compare
4e74ebc
to
16d0e3f
Compare
Squashed commits so that the fix and the test go together. |
16d0e3f
to
0715c75
Compare
@findepi Squashed, see if You like the commit message |
Prevent ViewReaders from passing view owner to ConnectorViewDefinition constructor, as it may fail the sanity check. The owner is added later on in HiveMetadata class Also add product test for `hive.hive-views.run-as-invoker` config option. Unit tests will not work here since we need to create a hive view not via Trino. Co-authored-by: Joe Chu <5282594+joechu1@users.noreply.github.com>
0715c75
to
49faf54
Compare
@skrzypo987 i used paste tense because was informing you about what i did. i only improved the commit title and authorship again |
Yeah, it appears that I forced pushed your changes Sorry about that. I shouldn't be allowed to use the --force option before the morning coffee. |
This is #12403 + product tests that should cover #12221 as well
Description
Non-technical explanation
Fix a bug when querying hive views with
hive.hive-views.run-as-invoker
set to trueRelease notes
I am not sure release notes are required since this feature was not working in the first place
( ) This is not user-visible and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: