>From Ian Maxon <[email protected]>: Attention is currently required from: Suryaa Charan Shivakumar.
Ian Maxon has posted comments on this change by Suryaa Charan Shivakumar. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968?usp=email ) Change subject: [ASTERIXDB-3542] Added Coordinate Reference System support at Metadata and ST Transform function ...................................................................... Patch Set 7: (11 comments) Patchset: PS7: looking very good overall, just an architectural concern and some questions and nitpicking File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968/comment/df4e6438_bbba1c84?usp=email : PS7, Line 5903: if (stmt instanceof CRSCreateStatement) { : stmtNamespace = ((CRSCreateStatement) stmt).getNamespace(); : } else if (stmt instanceof CRSDropStatement) { : stmtNamespace = ((CRSDropStatement) stmt).getNamespace(); : } could probably make this simpler by making a common CRSStatement or something since they both share this method File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/handlers/CRSStatementHandler.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968/comment/bfe9acac_01411562?usp=email : PS7, Line 115: // Validate WKT only for new CRS entries why's that? File asterixdb/asterix-doc/src/main/markdown/sqlpp/7_ddl_dml.md: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968/comment/788da539_0533ce4c?usp=email : PS7, Line 626: ### <a id="CRS_statements">Coordinate Reference System (CRS) Statements</a> very nice updating the docs atomically File asterixdb/asterix-doc/src/site/markdown/geo/functions.md: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968/comment/a8527846_865ad3c3?usp=email : PS7, Line 347: ### st_distance_spheroid ### same here File asterixdb/asterix-geo/src/main/java/org/apache/asterix/geo/evaluators/GeoFunctionTypeInferers.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968/comment/94838737_54eb164b?usp=email : PS7, Line 69: public static final class STTransformTypeInferer implements IFunctionTypeInferer { : @Override : public void infer(ILogicalExpression expr, IFunctionDescriptor fd, IVariableTypeEnvironment context, : CompilerProperties compilerProps) throws AlgebricksException { : throw new CompilationException(ErrorCode.COMPILATION_ERROR, expr.getSourceLocation(), : "ST_Transform requires a default dataverse to be set (USE <dataverse>)"); : } : : @Override : public void infer(ILogicalExpression expr, IFunctionDescriptor fd, IVariableTypeEnvironment context, : CompilerProperties compilerProps, Namespace defaultNamespace) throws AlgebricksException { : AbstractFunctionCallExpression fce = (AbstractFunctionCallExpression) expr; i think this is not the right way to go about this. there is always a namespace, implied or not. shoehorning this validation here kind of breaks this interface. IVariableTypeEnvironment should basically contain all the information needed to infer the type, which is derived from the database and namespace. so it's very weird to have a derivation of something from the namespace, and the namespace itself. i would try refactoring this to be analogous to how the FullTextContainsTypeInferer does it. you should be able to access the CRS dataset there and put it in the expression via setOpaqueParameters. it's a very similar pattern. File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataCache.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968/comment/8dc59fbe_72afba58?usp=email : PS7, Line 98: crsMap maybe just 'crs' here File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataNode.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968/comment/ba802188_d79ba059?usp=email : PS7, Line 3258: public static ITupleReference createIntTuple(int value) { : try { : ArrayTupleBuilder tupleBuilder = new ArrayTupleBuilder(1); : ISerializerDeserializer<AInt32> int32Serde = : SerializerDeserializerProvider.INSTANCE.getSerializerDeserializer(BuiltinType.AINT32); : AMutableInt32 aInt32 = new AMutableInt32(value); : tupleBuilder.addField(int32Serde, aInt32); : ArrayTupleReference tuple = new ArrayTupleReference(); : tuple.reset(tupleBuilder.getFieldEndOffsets(), tupleBuilder.getByteArray()); : return tuple; : } catch (HyracksDataException e) { : // This should never happen : throw new IllegalStateException("Failed to create search tuple", e); : } : } this doesn't seem to be used File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/CoordinateReferenceSystemEntity.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968/comment/4e7834f7_ae4eff06?usp=email : PS7, Line 1: /* this is kind of a nit but i would just rename this to CoordinateReferenceSystem or CRS to be consistent with the naming of other classes in this package. they're all entities but don't have Entity in the name File asterixdb/pom.xml: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968/comment/84f965ad_11b3d23e?usp=email : PS7, Line 131: nit: blank line https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968/comment/42ccd294_be02fd97?usp=email : PS7, Line 188: <exclude>**/*.wkt</exclude> i didn't see any .wkt files added in tests- are there any? and does wkt not have some way to add a license header? -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20968?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: Ia6e37080a581292744ddc9020b214926412c16ac Gerrit-Change-Number: 20968 Gerrit-PatchSet: 7 Gerrit-Owner: Suryaa Charan Shivakumar <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-CC: Ian Maxon <[email protected]> Gerrit-Attention: Suryaa Charan Shivakumar <[email protected]> Gerrit-Comment-Date: Thu, 16 Apr 2026 19:15:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
