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 7: (9 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/24521/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24521/3//COMMIT_MSG@37 PS3, Line 37: - VARIANT columns are serialized to their JSON representation in query : output (hs2-util, query-result-set), via the backend VariantSlotToJson : helper. > Good question, we could also pass as BINARY(s) and decode on client side by Yeah, later when clients can decode VARIANT binaries we can re-think that. http://gerrit.cloudera.org:8080/#/c/24521/3/be/src/exec/parquet/parquet-variant-column-reader.h File be/src/exec/parquet/parquet-variant-column-reader.h: http://gerrit.cloudera.org:8080/#/c/24521/3/be/src/exec/parquet/parquet-variant-column-reader.h@34 PS3, Line 34: VariantColumnReader > >If the data is corrupt, we usually don't guarantee correct behavior Filed IMPALA-15221. Since decoding was added separately I think we should fix it separately. I'll upload a CR soon for this. http://gerrit.cloudera.org:8080/#/c/24521/6/be/src/runtime/types.cc File be/src/runtime/types.cc: http://gerrit.cloudera.org:8080/#/c/24521/6/be/src/runtime/types.cc@108 PS6, Line 108: EQ(field_names[0], "me > this can't be true based on the DCHECKSs above (+ same for line below) Done http://gerrit.cloudera.org:8080/#/c/24521/6/be/src/runtime/types.cc@113 PS6, Line 113: is_binary_ > Can you rebase? This was removed in https://gerrit.cloudera.org/#/c/24567/ I'll rebase separately. http://gerrit.cloudera.org:8080/#/c/24521/6/be/src/service/hs2-util.cc File be/src/service/hs2-util.cc: http://gerrit.cloudera.org:8080/#/c/24521/6/be/src/service/hs2-util.cc@429 PS6, Line 429: column->stringVal.values.emplace_back(std::move(json)); : SetNullBit(output_row_idx, f > Will the client know that an error has happened? Switched to raising an error. http://gerrit.cloudera.org:8080/#/c/24521/6/be/src/service/query-result-set.cc File be/src/service/query-result-set.cc: http://gerrit.cloudera.org:8080/#/c/24521/6/be/src/service/query-result-set.cc@268 PS6, Line 268: } else { : DCHECK(type.IsStructType()); : const StructVal* struct_val = static_cast<const StructVal*>(value); : const SlotDescriptor* slot_d > Same as for HS2 case, will the user know that there was an error? Done. http://gerrit.cloudera.org:8080/#/c/24521/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test: http://gerrit.cloudera.org:8080/#/c/24521/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test@2 PS3, Line 2: ---- QUERY > thanks for the new tests! I missing one case: Added splilling order by http://gerrit.cloudera.org:8080/#/c/24521/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test: http://gerrit.cloudera.org:8080/#/c/24521/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test@189 PS6, Line 189: SELECT id, v FROM (SELECT id, v FROM trino_variant) t WHERE id IN (2, 4, 31); > A few more cases, mainly around views: Added new tests. http://gerrit.cloudera.org:8080/#/c/24521/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test@277 PS6, Line 277: > Can you add aggregation(SUM, SpilledPartitions)> 0 to verify that it actual Done -- 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: 7 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[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: Fri, 24 Jul 2026 11:36:44 +0000 Gerrit-HasComments: Yes
