Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24552 )
Change subject: IMPALA-15165: Add minimal C++ WKB serialization support ...................................................................... Patch Set 10: (5 comments) http://gerrit.cloudera.org:8080/#/c/24552/9/be/src/exprs/geo/wkb-format.h File be/src/exprs/geo/wkb-format.h: http://gerrit.cloudera.org:8080/#/c/24552/9/be/src/exprs/geo/wkb-format.h@41 PS9, Line 41: constexpr uint8_t WKB_BIG_ENDIAN = 0x00; : : // WKB geometry type codes (ISO 13249). : enum WkbType : uint32_t { : WKB_POINT = 1, : WKB_LINESTRING = 2, > nit: Why not an enum? Done http://gerrit.cloudera.org:8080/#/c/24552/9/be/src/exprs/geo/wkb-format.h@48 PS9, Line 48: = 4, > nit: this (and others) is lowerCamelCase, not UpperCamelCase. Done http://gerrit.cloudera.org:8080/#/c/24552/9/be/src/exprs/geo/wkb-format.h@77 PS9, Line 77: > nit: byte order constants could be consts/enum values for better readabilit Done http://gerrit.cloudera.org:8080/#/c/24552/9/be/src/exprs/geo/wkb-format.h@122 PS9, Line 122: > nit: Could be a more expressive name, indicating that byte order is swapped Done http://gerrit.cloudera.org:8080/#/c/24552/9/be/src/exprs/geo/wkb-serialization.h File be/src/exprs/geo/wkb-serialization.h: http://gerrit.cloudera.org:8080/#/c/24552/9/be/src/exprs/geo/wkb-serialization.h@59 PS9, Line 59: IsBboxInvalid > nit: lowerCamelCase instead of UpperCamelCase Done -- To view, visit http://gerrit.cloudera.org:8080/24552 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d5b2ac5d584e3de7712d8471acbc07d743a4de2 Gerrit-Change-Number: 24552 Gerrit-PatchSet: 10 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 31 Jul 2026 16:24:59 +0000 Gerrit-HasComments: Yes
