Zoltan Borok-Nagy 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 6: (9 comments) Thanks for the 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 I rather defer it for now as STRUCT and VARIANT only partially overlap, hence we would still end up having lots of IsVariant() branches. I can pursue it if you feel strong about it, but probably it would be better to refactor later as it might grow the scope of this patch significantly. http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/exec/parquet/hdfs-parquet-scanner.cc@2979 PS5, Line 2979: VARIANT column '$0 > The comment states that it might be a malformed or a shredded value Updated 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? We have other DCheck* methods as well in the codebase. I think it expresses the intent that it contains debug checks only. 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: } else { > it should go outside of the loop Done, however, since we use std::move(json) below, I'm not sure if we gain anything. http://gerrit.cloudera.org:8080/#/c/24521/5/be/src/service/hs2-util.cc@428 PS5, Line 428: literal "null" string, which would misrepresent corrupt data as > nit: not needed Done 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: // (no partial JSON leaks). > nit: VariantSlotToJson(void *value, std::stringstream& ss)? Done 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: lt); > What about a non-owning struct or a typedef/using for variant instead erasi Introduced VariantSlot http://gerrit.cloudera.org:8080/#/c/24521/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/24521/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1862 PS5, Line 1862: // created without a path. Maybe descriptors should have a path even in the > it's a code duplication from createStructTuplesAndSlotDescs, can it be coll Refactored a bit, see other comment about compound type. http://gerrit.cloudera.org:8080/#/c/24521/5/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/24521/5/tests/query_test/test_iceberg.py@2366 PS5, Line 2366: 19 > nit: where does this number come from? I just picked a small prime number that is less than the number of rows in the 'trino_variant' table. -- 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: 6 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: Tue, 21 Jul 2026 13:55:27 +0000 Gerrit-HasComments: Yes
