HIVE-28736:Remove DFS_URI authorization for CREATE_TABLE event with n… #5689
Conversation
…o explicit LOCATION
asf-ci-hive
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
|
|
@saihemanth-cloudera @ayushtkn Could you please help review this PR? Thank you |
There was a problem hiding this comment.
Thanx @rtrivedi12, had a quick pass, dropped some questions/comments.
One generic question, I couldn't find time to debug, does the HMS Translation layer kicks in before this or after. Just trying to see if things don't change post we made a decision here
| super(preEventContext); | ||
|
|
||
| super(preEventContext); | ||
| this.wh = preEventContext.getHandler().getWh(); |
There was a problem hiding this comment.
Why do we want to save it? It should be always available in preEventContext?
We can directly do below
preEventContext.getHandler().getWh().getDefaultTablePath(database, table.getTableName(), isExternalTable);
| String uri = getSdLocation(table.getSd()); | ||
| Database database = event.getDatabase(); | ||
| String uri = getSdLocation(table.getSd()); |
There was a problem hiding this comment.
I think there was some unconventional spacing, but can we keep the same thing to maintain redability
List<HivePrivilegeObject> ret = new ArrayList<>();
PreCreateTableEvent event = (PreCreateTableEvent) preEventContext;
Table table = event.getTable();
Database database = event.getDatabase();
String uri = getSdLocation(table.getSd());
| return ret; | ||
| } | ||
|
|
||
| boolean isExternalTable = table.getTableType().equalsIgnoreCase(TableType.EXTERNAL_TABLE.toString()); |
There was a problem hiding this comment.
does this work?
MetaStoreUtils.isExternalTable(table);
maybe we can use this directly if it does rather than storing in the variable
| boolean isExternalTable = table.getTableType().equalsIgnoreCase(TableType.EXTERNAL_TABLE.toString()); | ||
| String expectedTablePath = null; | ||
| try { | ||
| expectedTablePath = wh.getDefaultTablePath(database, table.getTableName(), isExternalTable).toString(); |
There was a problem hiding this comment.
I think we don't need to getTableName & things like that
expectedTablePath = wh.getDefaultTablePath(database, table).toString();
| try { | ||
| expectedTablePath = wh.getDefaultTablePath(database, table.getTableName(), isExternalTable).toString(); | ||
| } catch (MetaException e) { | ||
| LOG.warn("Got exception fetching Default table location for table " + table.getTableName(), e); |
There was a problem hiding this comment.
Can you use {} + add the database name as well
| Database database = event.getDatabase(); | ||
| String uri = getSdLocation(table.getSd()); | ||
| Database database = event.getDatabase(); | ||
| String uri = getSdLocation(table.getSd()); |
There was a problem hiding this comment.
can you match the indentation with above lines
| if (StringUtils.isNotEmpty(uri)) { | ||
| boolean isExternalTable = table.getTableType().equalsIgnoreCase(TableType.EXTERNAL_TABLE.toString()); | ||
| String expectedTablePath = null; | ||
| try { | ||
| expectedTablePath = wh.getDefaultTablePath(database, table.getTableName(), isExternalTable).toString(); | ||
| } catch (MetaException e) { | ||
| LOG.warn("Got exception fetching Default table location for table " + table.getTableName(), e); | ||
| } | ||
|
|
||
| // Skip DFS_URI for external tables and if managed table location is under default db path | ||
| if (!isExternalTable) { | ||
| if (StringUtils.isEmpty(expectedTablePath) || !uri.equals(expectedTablePath)) { |
There was a problem hiding this comment.
this part looks being duplicated, can you refactor into a method & return whether you need to add DFS_URI or not
| } | ||
|
|
||
| // Skip DFS_URI for external tables and if managed table location is under default db path | ||
| if (!isExternalTable) { |
There was a problem hiding this comment.
curious, trying to understand: In getInputObj you didn't had this !isExternalTable, but here you have it, why?
…o explicit LOCATION
What changes were proposed in this pull request?
This update ensures that DFS_URI authorization is triggered only when necessary during the CREATE TABLE command. Specifically:
For managed table creation, DFS_URI authorization is skipped if the location is under the default managed DB location.
For external table creation, DFS_URI authorization is skipped if the table location is under the default external DB location.
Why are the changes needed?
This prevents unnecessary DFS_URI authorization for tables created within default DB locations, and adding extra policies in Ranger for query execution from external clients like Spark.
Does this PR introduce any user-facing change?
Yes, After this change, users will no longer need the DFS_URI policy for tables that are created in the default managed DB location (for managed tables) or the default external DB location (for external tables).
Is the change a dependency upgrade?
No
How was this patch tested?
Changes were verified by running the CREATE table command from Spark sql.