>From Hussain Towaileb <hussai...@gmail.com>: Attention is currently required from: Ali Alsuliman, Ian Maxon, Michael Blow, Murtadha Hubail, Peeyush Gupta.
Hussain Towaileb has posted comments on this change by Hussain Towaileb. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272?usp=email ) Change subject: [ASTERIXDB-3634][EXT]: Add support to Iceberg ...................................................................... Patch Set 8: Code-Review+1 (15 comments) Patchset: PS3: > also: can we add the CATALOG railroad diagram to > asterix-doc/src/main/grammar/sqlpp. […] Yes, will add it in a follow up, good call. File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/handlers/CatalogStatementHandler.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/a00c5b61_61206ea8?usp=email : PS3, Line 226: // TODO(htowaileb): what is transaction state? > pretty sure it's a remnant from external indexing v. […] Got it, it's not needed for catalogs, removing it. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/86ffc501_a167c32f?usp=email : PS3, Line 240: for (Map.Entry<String, IAdmNode> me : withObjectNode.getFields()) { > feels like this logic can be extracted to a util outside of this class Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/6461178d_860c10d0?usp=email : PS3, Line 261: fieldName + "#" + i++ > do we actually store them this way in our metadata catalog? I believe we do, yes. It was related to a discussion on how to store an array of include/exclude properties, we went with this approach. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/8c827d67_810b139f?usp=email : PS3, Line 281: MetadataTransactionContext mdTxnCtx, MetadataProvider metadataProvider > unused? Used on the overridden method, cbas side File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/CatalogConfig.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/4b7d341e_8d78fa9c?usp=email : PS3, Line 35: IcebergCatalogSource > You can potentially use Optional<IcebergCatalogSource> here Done File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/catalog/ICatalogDetailsDecl.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/33d6ee02_e54265f2?usp=email : PS3, Line 21: ICatalogDetailsDecl > I know you are just following the existing pattern, but in this case, we > might not need this empty i […] Removed File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataCache.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/a4023677_3b17f63a?usp=email : PS6, Line 150: } > need to add lock and clear catalogs here Done File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataManager.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/a5f0069e_e575c59f?usp=email : PS3, Line 928: catalog > make the parameter name consist between get and drop (i.e., catalog or > catalogName). […] Done File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/bootstrap/CatalogEntity.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/62df4567_1a10e1fe?usp=email : PS3, Line 103: FIELD_NAME_CATALOG_NAME > we should seriously start considering adding a uuid for our entities to allow > renaming entities, but […] Will go over the metadata fields in a follow up and do the necessary after I get clearer picture, just so I don't break anything in this patch. File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/bootstrap/MetadataRecordTypes.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/931cf778_5cc0c38c?usp=email : PS3, Line 316: FIELD_NAME_LAST_REFRESH_TIME, : FIELD_NAME_TRANSACTION_STATE }, > Are you planning on adding a catalog refresh concept? if not, we you should > remove these two fields. Not adding that, removed. File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/IcebergCatalog.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/64b4e5f8_b5cf5264?usp=email : PS3, Line 26: 2 > why not 1? Done File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/IcebergCatalogDetails.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/4425515e_2e4758de?usp=email : PS3, Line 151: @Override : public String toString() { : ObjectMapper mapper = new ObjectMapper(); : try { : return mapper.writeValueAsString(toMap()); : } catch (JsonProcessingException e) { : LOGGER.log(Level.WARN, "Unable to convert map to json String", e); : return getClass().getSimpleName(); : } : } : : private Map<String, Object> toMap() { : Map<String, Object> map = new HashMap<>(); : map.put("adapter", adapter); : map.put("properties", properties); : map.put("lastRefreshTime", lastRefreshTime.toString()); : map.put("state", state.name()); : return map; : } > when does this get used instead of the other serialization form? Not sure actually, but I saw it everywhere, so I maintained it, @Murtadha might know about it. File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/MetadataLockUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/ab245357_cef16ffd?usp=email : PS3, Line 402: createCatalogBeing > nit: typo? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272/comment/e198267a_81995eac?usp=email : PS3, Line 404: acquireCatalogReadLock > Shouldn't this be write lock as this is used while creating a catalog? Done -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20272?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: If825dfc3241f72fcece03874662fc2dc8f553920 Gerrit-Change-Number: 20272 Gerrit-PatchSet: 8 Gerrit-Owner: Hussain Towaileb <hussai...@gmail.com> Gerrit-Reviewer: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Hussain Towaileb <hussai...@gmail.com> Gerrit-Reviewer: Ian Maxon <ima...@apache.org> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Murtadha Hubail <mhub...@apache.org> Gerrit-Reviewer: Peeyush Gupta <peeyush.gu...@couchbase.com> Gerrit-CC: Michael Blow <mb...@apache.org> Gerrit-Attention: Murtadha Hubail <mhub...@apache.org> Gerrit-Attention: Peeyush Gupta <peeyush.gu...@couchbase.com> Gerrit-Attention: Ian Maxon <ima...@apache.org> Gerrit-Attention: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Attention: Michael Blow <mb...@apache.org> Gerrit-Comment-Date: Mon, 22 Sep 2025 15:01:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Murtadha Hubail <mhub...@apache.org> Comment-In-Reply-To: Peeyush Gupta <peeyush.gu...@couchbase.com> Comment-In-Reply-To: Ian Maxon <ima...@apache.org> Comment-In-Reply-To: Michael Blow <mb...@apache.org>
