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

Pushdown predicates fully in BigQuery connector #19699

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Nov 10, 2023

Description

Pushdown predicates fully in BigQuery connector

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Nov 10, 2023
@github-actions github-actions bot added the bigquery BigQuery connector label Nov 10, 2023
@ebyhr ebyhr force-pushed the ebi/bigquery-filter-pushdown branch 3 times, most recently from bb04138 to 3b23202 Compare November 11, 2023 00:43
Comment on lines +145 to +147
tableConstraint.getDomains().ifPresent(domains -> domains.keySet().stream()
.map(column -> ((BigQueryColumnHandle) column).getName())
.forEach(projectedColumnsNames::add));
Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise, TestBigQueryAvroConnectorTest.testEmptyProjectionView throws the following error:

io.grpc.StatusRuntimeException: INVALID_ARGUMENT: request failed: Query error: Unrecognized name: regionkey at [1:105]

@ebyhr ebyhr marked this pull request as ready for review November 11, 2023 01:56
@ebyhr ebyhr requested review from wendigo and hashhar November 11, 2023 01:56
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

the optional to non-optional API change makes it slightly hard to review but I can't think of ways to avoid that at the moment.

Do we have tests already which result is remainingFilters being present?

@ebyhr ebyhr force-pushed the ebi/bigquery-filter-pushdown branch from 3b23202 to cfb86d5 Compare November 13, 2023 23:02
@ebyhr
Copy link
Member Author

ebyhr commented Nov 13, 2023

Do we have tests already which result is remainingFilters being present?

We don't have the test as far as I confirmed. Added BaseBigQueryConnectorTest#testPredicatePushdown() test.

@ebyhr ebyhr force-pushed the ebi/bigquery-filter-pushdown branch 2 times, most recently from 0ef7d12 to 7c4999a Compare November 15, 2023 02:03
@ebyhr ebyhr force-pushed the ebi/bigquery-filter-pushdown branch from 7c4999a to dc5a296 Compare November 16, 2023 07:47
@ebyhr
Copy link
Member Author

ebyhr commented Nov 16, 2023

Rebased on master to resolve conflicts.

@ebyhr ebyhr requested a review from hashhar November 16, 2023 07:47
@Test
public void testPredicatePushdown()
{
testPredicatePushdown("true", "true", true);
Copy link
Member

Choose a reason for hiding this comment

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

i was thinking of tests which had two predicates, one which could be pushed down and the other which gets retained.

This one is useful too since it was missing entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's improve the test coverage in follow-up.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % suggestion (feel free to ignore since it might not provide additional coverage/confidence).

@ebyhr ebyhr merged commit 5aba093 into master Nov 16, 2023
16 of 17 checks passed
@ebyhr ebyhr deleted the ebi/bigquery-filter-pushdown branch November 16, 2023 12:41
@github-actions github-actions bot added this to the 434 milestone Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery BigQuery connector cla-signed
Development

Successfully merging this pull request may close these issues.

2 participants