Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/24521 )
Change subject: IMPALA-15052: Add read support for unshredded VARIANT values ...................................................................... Patch Set 5: (7 comments) Solid patch, Zoltan! I'm posting the first batch of comments. http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/exec/parquet/hdfs-parquet-scanner.cc File be/src/exec/parquet/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/exec/parquet/hdfs-parquet-scanner.cc@2963 PS5, Line 2963: IsVariantType I'm thinking about whether struct and variant could be handled as a common object, something like "compound" object. The change affect a lot of places where branching now consists of (struct or variant). http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/exec/parquet/hdfs-parquet-scanner.cc@2979 PS5, Line 2979: Unshredded VARIANT The comment states that it might be a malformed or a shredded value http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/runtime/types.cc File be/src/runtime/types.cc: http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/runtime/types.cc@104 PS5, Line 104: void ColumnType::DCheckVariantShape() const { nit: DCheckVariantShape sounds odd, what about just CheckVariantShape? http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/service/hs2-util.cc File be/src/service/hs2-util.cc: http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/service/hs2-util.cc@419 PS5, Line 419: string json; it should go outside of the loop http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/service/hs2-util.cc@428 PS5, Line 428: Log bounded to avoid flooding when an entire column is corrupt. nit: not needed http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/service/query-result-set.cc File be/src/service/query-result-set.cc: http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/service/query-result-set.cc@264 PS5, Line 264: Status status = VariantSlotToJson(value, &json); nit: VariantSlotToJson(void *value, std::stringstream& ss)? http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/util/variant-util.cc File be/src/util/variant-util.cc: http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/util/variant-util.cc@45 PS5, Line 45: void* What about a non-owning struct or a typedef/using for variant instead erasing the type fully? -- To view, visit http://gerrit.cloudera.org:8080/24521 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2f8a7c9b1d4e5f6a0c3b8d7e9f1a2b4c6d8e0f1 Gerrit-Change-Number: 24521 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 20 Jul 2026 15:38:14 +0000 Gerrit-HasComments: Yes
