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 3: (9 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 Probably not a popular opinion, but I would recommend against this approach (based on my bad experience with complex types) in this early stage of VARIANT support, both in FE and BE. IMO the best approach is to do an implementation that is as simple as possible, even if it is sub-optimal and write a more efficient later, when it becomes possible to benchmark it. Feel free to disagree with the things below :) FE: Does being treated as a struct ha any effect on the code? on the first glance my impression was that VARIANTs are treated as a scalar in the frontend. BE: On the first try I would use a single StringValue slot and encode variants to it somehow like for BINARY. An example is to start with an int32 for the length of metadata then copy both metadata and value after it. For GEOMETRY it took me around 100 rows of trivial c++ code to do this plumbing, and it allowed passing the new type as argument and returning it from functions. No restrictions for materialization or codegen. My guess is that nested variants would also work (but it would make sense to forbid them anyway to simplify testing). The current 2 StringValue approach is much more efficient during scanning, as the pointers can point to the source data, but other parts of the query don't get this advantage, the data is duplicated during exchanges or when writing to BufferedTupleStream, for example to use on build side of join, or on the probe side when spilled. Also, keeping the deduplication will be hard when returning results from VARIANT returning functions. It is not clear what internal representation is ideal. There are several factors: - shredded vs unshredded - the query uses the whole variant vs just selected fields - the query is non selective/selective with predicates on variant fields / without predicates on variant fields - the metadata is uniform vs varied Using the Parquet variant encoding internally and avoiding the copy from the original buffer is mainly useful if the query is not selective and the whole variant needs to be returned. Otherwise late materialization and allowing to keep only the used members becomes more important. If we know the limited number of used members, we don't even need to keep their names, we can assign a fix numbers to them. Referencing the data buffer looks free, but for selective queries it can lead to accumulating multiple decompressed data pages attached to the buffer - we could have released them if the data we needed was copied. 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 encoded and convert on client side? 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.java, even if they return an error currently. Note that you can create a given type with cast(null as variant) without having any function that returns a variant. http://gerrit.cloudera.org:8080/#/c/24521/3//COMMIT_MSG@46 PS3, Line 46: - VARIANT raises an AnalysisException in ORDER BY, GROUP BY, : SELECT DISTINCT, UNION/INTERSECT/EXCEPT, CASE/DECODE, IS NULL, : analytic PARTITION BY/ORDER BY, aggregate DISTINCT, INSERT, and : COMPUTE STATS. Does variant support = / > / < etc? I guess no, and most of this missing features directly come from not supporting basic operators. I would separate the missing features below: a. things not supported by design, e.g. ORDER BY makes no sense without < b. things we plan do support. just not ready yet, for example INSERT / CASE/ IS NULL (probably because of functions can't get/return VARIANT yet) c. things that may be supported, but the design is not clear,. e.g COMPUTE STATS http://gerrit.cloudera.org:8080/#/c/24521/3//COMMIT_MSG@50 PS3, Line 50: - UNION ALL of a variant column is supported. is this true for non-pass-through UNION ALL? commented about this in the test too 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 (+ int WITH and without WITH) can return VARIANTS? this was a massive pain point in complex type support http://gerrit.cloudera.org:8080/#/c/24521/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test@161 PS3, Line 161: SELECT id, v FROM trino_variant WHERE id < 3 : UNION ALL : SELECT id, v FROM trino_variant WHERE id = 2 : ORDER BY id; This is a pass-through UNION ALL - can you also try a non-pass-through one? For example use id-1 in one branch, id+1 in the other. http://gerrit.cloudera.org:8080/#/c/24521/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test@185 PS3, Line 185: GROUP BY expression cannot be used on complex types nit: the error message complains about complex types, not variant 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_variant test_iceberg.py is already huge, variant could get its own file -- 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: 3 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-Comment-Date: Tue, 07 Jul 2026 14:10:57 +0000 Gerrit-HasComments: Yes
