Balazs Hevele has posted comments on this change. ( http://gerrit.cloudera.org:8080/24552 )
Change subject: IMPALA-15165: Add minimal C++ WKB serialization support ...................................................................... Patch Set 9: (5 comments) Looks good, only added some nits. 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 uint32_t WKB_POINT = 1; : constexpr uint32_t WKB_LINESTRING = 2; : constexpr uint32_t WKB_POLYGON = 3; : constexpr uint32_t WKB_MULTIPOINT = 4; : constexpr uint32_t WKB_MULTILINESTRING = 5; : constexpr uint32_t WKB_MULTIPOLYGON = 6; nit: Why not an enum? http://gerrit.cloudera.org:8080/#/c/24552/9/be/src/exprs/geo/wkb-format.h@48 PS9, Line 48: readUint32 nit: this (and others) is lowerCamelCase, not UpperCamelCase. http://gerrit.cloudera.org:8080/#/c/24552/9/be/src/exprs/geo/wkb-format.h@77 PS9, Line 77: 0x00 nit: byte order constants could be consts/enum values for better readability. http://gerrit.cloudera.org:8080/#/c/24552/9/be/src/exprs/geo/wkb-format.h@122 PS9, Line 122: swap nit: Could be a more expressive name, indicating that byte order is swapped. 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 -- 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: 9 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, 17 Jul 2026 07:13:14 +0000 Gerrit-HasComments: Yes
