Till Westmann has posted comments on this change. Change subject: ASTERIXDB-1371 - Define new datatype 'geometry' user model changes: Add new builtin type 'geometry' storage format changes: no interface changes: no ......................................................................
Patch Set 1: (5 comments) https://asterix-gerrit.ics.uci.edu/#/c/1838/1//COMMIT_MSG Commit Message: PS1, Line 8: user please add an empty line between the subject and the body https://asterix-gerrit.ics.uci.edu/#/c/1838/1/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/geojson/datatype/datatype_definition.1.ddl.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/geojson/datatype/datatype_definition.1.ddl.sqlpp: PS1, Line 27: geometry > Should this be allowed? I think the type name should be a reserved word. It I think that it's ok as type identifiers and field names are just identifiers, i.e. they are not identified as tokens by the lexer. But for the example it would be nice to use different names to be able to distinguish between them without much thought. https://asterix-gerrit.ics.uci.edu/#/c/1838/1/asterixdb/asterix-app/src/test/resources/runtimets/results/geojson/datatype/datatype.1.adm File asterixdb/asterix-app/src/test/resources/runtimets/results/geojson/datatype/datatype.1.adm: Line 1 > Empty file? It is the correct result for the first query - but usually we have these results with a license header (even though I'm not sure that there's anything to license in an empty file - it probably makes RAT happy ... https://asterix-gerrit.ics.uci.edu/#/c/1838/1/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml File asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml: Line 22: <!ENTITY GeoQueries SYSTEM "queries_sqlpp/geojson/GeoJSONQueries.xml"> > Not sure why this should be a separate test suite? I think because it's the start of a test suite. The advantage of separating these into different files, it that is is easier to run individual parts. It's the same as the other entities declared above. But I think that it might be better to to all those at the top to make it easier to connect the entity declaration to it's use. https://asterix-gerrit.ics.uci.edu/#/c/1838/1/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/BuiltinType.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/BuiltinType.java: PS1, Line 708: type > All other types are using their typetag here (captial). For example, check Agreed. -- To view, visit https://asterix-gerrit.ics.uci.edu/1838 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: If32dbfcc11350a81e6a64f6abeec0595aa7872d2 Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Riyafa Abdul Hameed <riyafa...@cse.mrt.ac.lk> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Taewoo Kim <wangs...@gmail.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-HasComments: Yes
