-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Conversation
bb04138
to
3b23202
Compare
tableConstraint.getDomains().ifPresent(domains -> domains.keySet().stream() | ||
.map(column -> ((BigQueryColumnHandle) column).getName()) | ||
.forEach(projectedColumnsNames::add)); |
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.
Otherwise, TestBigQueryAvroConnectorTest.testEmptyProjectionView
throws the following error:
io.grpc.StatusRuntimeException: INVALID_ARGUMENT: request failed: Query error: Unrecognized name: regionkey at [1:105]
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 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?
3b23202
to
cfb86d5
Compare
We don't have the test as far as I confirmed. Added |
0ef7d12
to
7c4999a
Compare
7c4999a
to
dc5a296
Compare
Rebased on master to resolve conflicts. |
@Test | ||
public void testPredicatePushdown() | ||
{ | ||
testPredicatePushdown("true", "true", true); |
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 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.
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.
Let's improve the test coverage in follow-up.
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.
LGTM % suggestion (feel free to ignore since it might not provide additional coverage/confidence).
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.