Csaba Ringhofer 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: (13 comments) http://gerrit.cloudera.org:8080/#/c/24521/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24521/3//COMMIT_MSG@17 PS3, Line 17: two > I still think the 2 StringValue slot approach provides us the required flex ack 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. > I don't know, but with current HS2 protocol this is our only option. Good question, we could also pass as BINARY(s) and decode on client side by pulling in Parquet lib. This is a dilemma for me with GEOMETRY - clients that have some geo lib could use WKB best, but it is not useful for clients that can't decode it, and readable text is better in that case. It may make sense to add a query option in the future to switch between encoded and string format, but I agree that json is the best for now. 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 > Integrity checking of VARIANT values requires doing the following: >If the data is corrupt, we usually don't guarantee correct behavior Looked into VariantValue, and I don't see any protection against corrupt offsets, so it can index out of bounds, not just return incorrect results. Also no protection against corrupt offsets in metadata's dictionary. >I think it is prohibitively expensive to do it eagerly for every variant. For metadata where we expect dictionary encoding this seems questionable to me, as the same metadata (validated once) is likely to be read multiple times, and the necessary protection against corrupt values adds overhead to each read. For data I see the benefits of lazy validation. I am ok with not validating eagerly, but then we should protect against corrupt offsets. I didn't look into it that deeply, but it seems relevant event with current patch, as conversion do json can read to unrelated memory. btw bumped into a related PR in parquet-java: https://github.com/apache/parquet-java/pull/3562 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: field_names.size() < 2 this can't be true based on the DCHECKSs above (+ same for line below) 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/ 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: LOG_EVERY_N(WARNING, 100) << "Failed to decode VARIANT value to JSON: " : << status.GetDetail(); Will the client know that an error has happened? This seems a new situation, all other type expect the serialization to always succeed. 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: // A decode failure on a materialized variant slot indicates corruption; don't : // discard it silently. Emit JSON null as a last resort, bounded-logging the error. : LOG_EVERY_N(WARNING, 100) << "Failed to decode VARIANT value to JSON: " : << status.GetDetail(); Same as for HS2 case, will the user know that there was an error? http://gerrit.cloudera.org:8080/#/c/24521/3/common/thrift/Types.thrift File common/thrift/Types.thrift: http://gerrit.cloudera.org:8080/#/c/24521/3/common/thrift/Types.thrift@98 PS3, Line 98: onal list< > Updated the comment for now. Let's have a deeper design discussion about va ack 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 ideas, added these. thanks for the new tests! I missing one case: >spilling in joins and sorting no spilling order by was added 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: - same variant col is returned to times in select list - same for variant that comes from view - nested views (e.g. inline view using HMS view) - same WITH view used multiple times in query If I remember correctly these revealed a few issues when working on complex types and unification of multiple uses of a struct in query. http://gerrit.cloudera.org:8080/#/c/24521/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test@277 PS6, Line 277: SET_DENY_RESERVATION_PROBABILITY Can you add aggregation(SUM, SpilledPartitions)> 0 to verify that it actually spills? The join looks fragile to me, an optimization in the future may optimize out FROM (SELECT 31 AS pid) http://gerrit.cloudera.org:8080/#/c/24521/3/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/24521/3/tests/query_test/test_iceberg.py@2459 PS3, Line 2459: test_v3_optimiz > Currently VARIANT is an Iceberg V3-only feature. I'd rather keep it here fo ack http://gerrit.cloudera.org:8080/#/c/24521/3/tests/query_test/test_iceberg.py@2461 PS3, Line 2461: > 1. Added batch_sizes test dimension to the whole V3 suite ack -- 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: 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: Thu, 23 Jul 2026 15:28:29 +0000 Gerrit-HasComments: Yes
