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.

Sign up for GitHub

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

Jump to bottom

Filter table columns in bulk #18956

Merged
merged 1 commit into from
Sep 9, 2023
Merged

Filter table columns in bulk #18956

merged 1 commit into from
Sep 9, 2023

Conversation

findepi
Copy link
Member

@findepi findepi commented Sep 7, 2023

Refactor information_schema.columns handling code.

@cla-bot cla-bot bot added the cla-signed label Sep 7, 2023
@github-actions github-actions bot added tests:hive delta-lake Delta Lake connector hive Hive connector labels Sep 7, 2023
@findepi findepi force-pushed the findepi/delta-filter branch 2 times, most recently from 6a6aa5c to 4517213 Compare September 7, 2023 14:55
electrum
electrum requested changes Sep 7, 2023
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

We should deprecate the old method

kokosing
kokosing approved these changes Sep 8, 2023
View reviewed changes
...-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ReadOnlySystemAccessControl.java
@@ -153,6 +153,12 @@ public Set<String> filterColumns(SystemSecurityContext context, CatalogSchemaTab
return columns;
}

@Override
public Map<SchemaTableName, Set<String>> filterColumns(SystemSecurityContext context, String catalogName, Map<SchemaTableName, Set<String>> tableColumns)
Copy link
Member

Choose a reason for hiding this comment

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

Should we deprecate the other filterColumns method?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

lib/trino-array/pom.xml Outdated
@@ -13,7 +13,6 @@

<properties>
<air.main.basedir>${project.parent.basedir}</air.main.basedir>
<air.compiler.fail-warnings>true</air.compiler.fail-warnings>
Copy link
Member

Choose a reason for hiding this comment

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

Separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure thing!

Copy link
Member Author

Choose a reason for hiding this comment

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

done! -- #18971

This was referenced Sep 8, 2023
Merged
Merged
@findepi findepi force-pushed the findepi/delta-filter branch 2 times, most recently from 4274fd1 to 3dc8f85 Compare September 8, 2023 12:33
@findepi findepi changed the title Simplify redirection handling and filter table columns in bulk Filter table columns in bulk Sep 8, 2023
lukasz-walkiewicz
lukasz-walkiewicz reviewed Sep 8, 2023
View reviewed changes
core/trino-main/src/main/java/io/trino/security/AccessControlManager.java
requireNonNull(tableColumns, "tableColumns is null");

Set<SchemaTableName> filteredTables = filterTables(securityContext, catalogName, tableColumns.keySet());
if (!filteredTables.equals(tableColumns.keySet())) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: if seems redundant as equals uses contains as well so in some cases it will execute contains twice for some elements

Copy link
Member Author

@findepi findepi Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i would expect equals to invoke contains only if sets are equal (size chcked first?), and there is no second call then for given element

core/trino-main/src/main/java/io/trino/metadata/MetadataListing.java
});
List<ColumnMetadata> columns = metadata.getTableMetadata(session, targetTableHandle).getColumns();

Set<String> allowedColumns = accessControl.filterColumns(
Copy link
Member

Choose a reason for hiding this comment

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

Can all redirected tables be collected and batched in a single accessControl.filterColumns call?

Copy link
Member Author

Choose a reason for hiding this comment

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

they would need to be grouped by catalog, but yes, that's possible

note that there are also other deficiencies around redirected tables handling. this requires test coverage and is not the part i am focusing on

@findepi findepi merged commit 3ec5767 into master Sep 9, 2023
101 checks passed
@findepi findepi deleted the findepi/delta-filter branch September 9, 2023 19:41
@github-actions github-actions bot added this to the 427 milestone Sep 9, 2023
@colebow colebow mentioned this pull request Sep 12, 2023
Merged
@colebow colebow added the no-release-notes This pull request does not require release notes entry label Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasz-walkiewicz lukasz-walkiewicz lukasz-walkiewicz left review comments

@kokosing kokosing kokosing approved these changes

@losipiuk losipiuk Awaiting requested review from losipiuk

@hashhar hashhar Awaiting requested review from hashhar

@djsstarburst djsstarburst Awaiting requested review from djsstarburst

@electrum electrum Awaiting requested review from electrum

Assignees
No one assigned
Labels
cla-signed delta-lake Delta Lake connector hive Hive connector no-release-notes This pull request does not require release notes entry
Milestone
427
Development

Successfully merging this pull request may close these issues.
5 participants
@findepi @electrum @kokosing @lukasz-walkiewicz @colebow