>From Hussain Towaileb <[email protected]>: Attention is currently required from: Peeyush Gupta.
Hussain Towaileb has posted comments on this change by Hussain Towaileb. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478?usp=email ) Change subject: [ASTERIXDB-3634][EXT]: Add support to Iceberg pt.2 ...................................................................... Patch Set 6: Code-Review+1 (12 comments) File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/awsclient/MyCustomAwsClientFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/c84d2a2a_1df1d628?usp=email : PS5, Line 42: AwsClientFactory > There are already classes like org.apache.iceberg.aws. […] I will have to give them another pass eventually to see if it is feasible, I am implementing this to ensure we support all of our authentication methods, not just AssumeRoleAwsClientFactory https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/11541bd9_fbef232c?usp=email : PS5, Line 42: MyCustomAwsClientFactory > The name could be EnsureCloseAWSClientFactory or something related. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/d4514e70_9c4541b7?usp=email : PS5, Line 47: proxyGlueClient > Ignore this for now, this is to find a way to close the inner most clients > (STS client when assuming […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/dd959e23_4b6eecc3?usp=email : PS5, Line 69: // if (proxyGlueClient == null) { : // GlueClient delegate = (GlueClient) glueClients.getConsumingClient(); : // proxyGlueClient = (GlueClient) Proxy.newProxyInstance( : // GlueClient.class.getClassLoader(), : // new Class<?>[]{GlueClient.class}, : // new GlueClientInvocationHandler(delegate)); : // } : // return proxyGlueClient; > remove commented out code Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/23643925_44e16161?usp=email : PS5, Line 79: @Override : public KmsClient kms() { : return null; : } : : @Override : public software.amazon.awssdk.services.dynamodb.DynamoDbClient dynamo() { : return null; : } > What are these functions for? Is returning null here ok? Yes, this creates the clients needed by the underlying operations, in our case, it's only Glue and S3 that we need, the others can be null. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/326581d3_8cb8c9db?usp=email : PS5, Line 112: private class GlueClientInvocationHandler implements InvocationHandler { : private final GlueClient delegate; : private volatile boolean closed = false; : : GlueClientInvocationHandler(GlueClient delegate) { : this.delegate = delegate; : } : : @Override : public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { : String name = method.getName(); : if ("close".equals(name)) { : if (!closed) { : closed = true; : try { : delegate.close(); : } finally { : // Close both sets; no-op if not built or already closed : AwsUtils.closeClients(glueClients); : AwsUtils.closeClients(s3Clients); : } : } : return null; : } : return method.invoke(delegate, args); : } : } > This seems to be unused. […] Yes. So I can close the iceberg client, which will close the clients in this class (i.e, glue and s3 clients). However, when using assume role authentication, we have under the hood another STS client and AssumedRoleCredentialsProvider that need to be closed. So the aim is to listen to when Glue or S3 clients are closed, and trigger closing the underlying other clients, not implemented yet. File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/aws/delta/IcebergFileRecordReader.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/cd5b432d_771243eb?usp=email : PS5, Line 19: delta > I think this should be in the iceberg aws. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/fbafd20d_0ca90b9e?usp=email : PS5, Line 146: } > do we need to close the catalog here? Yes we do, done. File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/aws/delta/converter/IcebergConverterContext.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/fab582d1_24329def?usp=email : PS5, Line 19: aws.delta.converter > I think this should be in the iceberg aws.iceberg.converter package or just > aws. […] Done, was copy/pasted since it's similar logic. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/06fbcaea_3df206b5?usp=email : PS5, Line 57: DeltaOptions.DECIMAL_TO_DOUBLE, > It will be better to create a separate IcebergOptions class and then using > that class here. […] Another copy/paste, fixed. I agree, on having a CommonOptions, I can do it in a follow up. File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/IcebergParquetDataParser.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/c657ad95_475cf5bd?usp=email : PS5, Line 222: case LIST: : Types.ListType listType = fieldType.asListType(); : parseArray(listType.elementType(), listType.isElementOptional(), (List<?>) value, out); : return; > While reading a list we are getting a array index out of bounds error. Only primitive types have been fully implemented, complex types and dates are not yet done, will work on in a follow up. File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/IndexUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/89db4467_e692964c?usp=email : PS5, Line 389: NoOpIcebergTableFilterEvaluatorFactory.INSTANCE > I think this is where the filter work would begin for pruning and filtering Done -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I531c613fe061367b2cb5c187bc985b39d6de17d1 Gerrit-Change-Number: 20478 Gerrit-PatchSet: 6 Gerrit-Owner: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Peeyush Gupta <[email protected]> Gerrit-Attention: Peeyush Gupta <[email protected]> Gerrit-Comment-Date: Wed, 10 Dec 2025 02:35:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Peeyush Gupta <[email protected]> Comment-In-Reply-To: Hussain Towaileb <[email protected]>
