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 4: (14 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@17 PS3, Line 17: two I still think the 2 StringValue slot approach provides us the required flexibility and efficiency. Also, if we ever want to allow reading the raw bytes of 'metadata' or 'value' (e.g. debugging reasons) it will be easier to accomplish. > btw do you know what encoding is used in the example files? metadata is dictionary encoded, value is plain encoded. Newer trino uses DELTA_LENGTH_BYTE_ARRAY for the value part. 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. > Do you know if other engines also treat it this way, or they pass it encode I don't know, but with current HS2 protocol this is our only option. http://gerrit.cloudera.org:8080/#/c/24521/3//COMMIT_MSG@44 PS3, Line 44: - The VARIANT keyword is recognized by the parser but rejected as a : user-specified column type or CAST target (CREATE/ALTER TABLE, CAST). > It would be nice to add the usual analyzer tests, e.g. in AnalyzeExprsTest. Added a few tests, but currently they aren't too useful as the cast(null as VARIANT) already fails. http://gerrit.cloudera.org:8080/#/c/24521/3//COMMIT_MSG@46 PS3, Line 46: : Unsupported operations by design : - ORDER BY, GROUP BY, SELECT DISTINCT, UNION/INTERSECT/EXCEPT, CASE/DECODE : analytic PARTI > Does variant support = / > / < etc? I guess no, and most of this missing fe Added these sections. About COMPUTE STATS, we won't have NDV for VARIANTs, but we should have size stats and num NULLs. http://gerrit.cloudera.org:8080/#/c/24521/3//COMMIT_MSG@50 PS3, Line 50: > is this true for non-pass-through UNION ALL? commented about this in the te Yes, added test. 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: * checking metadata header bit fields * dictionary size check * boundary check of each dictionary offset * if sorted_strings = true, verify that the dictionary elements are indeed sorted * ensure that there are no duplicate keys * recursively traverse the 'value' part and do a type-specific validation I think it is prohibitively expensive to do it eagerly for every variant. AFAIK every engine does this lazily, and only for the parts that is required to read. This behavior is also inline with how Impala behaves in other contexts (e.g. we don't do full Parquet metadata validation). > but this can make it harder to diagnose where the corrupt variant comes from Users can figure it out using the values in other columns. Later we plan to push down variant_get() expressions to the scanners, so it will be easier to provide that information. > or Impala interprets some things incorrectly If the data is corrupt, we usually don't guarantee correct behavior 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< > how could this work? isn't shredding completely up to the Parquet layer, so Updated the comment for now. Let's have a deeper design discussion about variant projections later. http://gerrit.cloudera.org:8080/#/c/24521/3/testdata/data/iceberg_test/iceberg_v3/trino_variant/metadata/v1.metadata.json File testdata/data/iceberg_test/iceberg_v3/trino_variant/metadata/v1.metadata.json: http://gerrit.cloudera.org:8080/#/c/24521/3/testdata/data/iceberg_test/iceberg_v3/trino_variant/metadata/v1.metadata.json@1 PS3, Line 1: { > new table data could be mentioned in https://github.com/apache/impala/blob/ 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@1 PS3, Line 1: ==== > one thing I miss from the tests are views - can you check both HMS, inline Thanks, HMS view revealed a bug. http://gerrit.cloudera.org:8080/#/c/24521/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test@2 PS3, Line 2: ---- QUERY > It would be nice to exercise scenarios that deep copy variants. Thanks for ideas, added these. http://gerrit.cloudera.org:8080/#/c/24521/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test@161 PS3, Line 161: UNION ALL : SELECT id, v FROM trino_variant WHERE id = 2 : ORDER BY id; : ---- RESULTS > This is a pass-through UNION ALL - can you also try a non-pass-through one? Done http://gerrit.cloudera.org:8080/#/c/24521/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test@185 PS3, Line 185: BIGINT, STRING > nit: the error message complains about complex types, not variant Done 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 > test_iceberg.py is already huge, variant could get its own file Currently VARIANT is an Iceberg V3-only feature. I'd rather keep it here for now. http://gerrit.cloudera.org:8080/#/c/24521/3/tests/query_test/test_iceberg.py@2461 PS3, Line 2461: > A few gaps related to the tests: 1. Added batch_sizes test dimension to the whole V3 suite 2. If multiple tables are involved in a query then the plan is distributed. -- 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: 4 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: Thu, 16 Jul 2026 16:16:59 +0000 Gerrit-HasComments: Yes
