Adding change for big query substrait plan #3122
Conversation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
1 similar comment
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
1 similar comment
|
✅ I finished the code review, and didn't find any security or code quality issues. |
|
hi; this change is touching too many packages at the same time; reverting it in case of a bug will be really problematic; can you please split them out to three if possible; or at least the changes to the two different plugins to be done separately. thanks. In the mean time I'll do a quick review here. |
| */ | ||
| public boolean offerValue(String fieldName, int row, Object value, boolean hasQueryPlan) | ||
| { | ||
| if (!hasQueryPlan && !constraintEvaluator.apply(fieldName, value)) { |
There was a problem hiding this comment.
the reason I'm asking is that all other methods here check only for constraintEvaluator.apply being true; I guess why the new condition is needed?
There was a problem hiding this comment.
just finished going through the unit test; it seems you only want this method to work with a plan and not constraint object. my concern is that we are expecting other methods to work with both except for this one method,
| try (BlockAllocatorImpl allocator = new BlockAllocatorImpl()) { | ||
| int resolvedSerDeVersion = SerDeVersion.SERDE_VERSION; | ||
| byte[] allInputBytes = com.google.common.io.ByteStreams.toByteArray(inputStream); | ||
| logger.info("allInputBytes: '{}'", new String(allInputBytes, StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
should this be debug to reduce noise?
| private static final String SECRET_ARN_KEY = "secret_arn"; | ||
| private static final String AUTH_DB_KEY = "AUTHENTICATION_DATABASE"; | ||
|
|
||
| // JSON credential field names |
There was a problem hiding this comment.
should all these be here; athena-docdb/src/main/java/com/amazonaws/athena/connectors/docdb/DocDBEnvironmentProperties.java ?
2 participants
Description of changes:
As part of query federation feature we have added support for Substrait query plan in the GoogleBigQuery connector to use the plan and execute the queries.
We have also made updates in Athena Federation SDK in the SubstraitFunctionParser in order to add additional support for the following:
Existing methods unchanged: All current functionality preserved
Additive enhancement: New method doesn't affect existing code paths
We have made sure that these are backwards compatible.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.