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: (6 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 > We can re-think the FE-side (scalar vs complex), but at the BE-side the cur >With "an int32 for the length of metadata then copy both metadata and value >after it" complicates things too much I agree with the copying, but not with the complication. Maybe I over-worry this because I don't see the end result of the design where limitations like no codegen are solved. (only glimpsed into your next patch with get methods, not sure if it solves that). The current design looks pretty elegant by relying on structs and avoiding copies, but the duality of sometimes treating them as a separate types, sometime as a single type adds some weirdness. I also trust structs much less than plain string slots, they are much less tested and used, so I am more picky about testing than if variant was piggy-backing on string. >What do we do in case of other PLAIN-encoded data? Probably the better >approach is to adaptively copy such binary/string data. yes, this is relevant for other data too, not just variant btw do you know what encoding is used in the example files? 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 An important question is whether to validate variants. Validating on usage (e.g. to json conversion) can work, but this can make it harder to diagnose where the corrupt variant comes from, it may not be possible the identify the file. IMO the ideal way would be to validate now, probably with the possibility of disabling with a flag. This type is pretty new, so possibly some writers still have bugs, or Impala interprets some things incorrectly. 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: unshredded how could this work? isn't shredding completely up to the Parquet layer, so can vary from file to file (or maybe row group to rowg group)? or Iceberg stores the shredded members per file in manifests? as we discussed offline, I think what we could do is collecting member accesses with const names during planning, and possibly project these out 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: {"format-version":3,"table-uuid":"a92ad483-a5eb-4c48-bcec-857cb4550259","location":"hdfs://localhost:20500/test-warehouse/trino_variant-8e46befd49c84eceb8028a4558c1f336","last-sequence-number":1,"last-updated-ms":1782726955568,"last-column-id":3,"current-schema-id":0,"schemas":[{"type":"struct","schema-id":0,"fields":[{"id":1,"name":"id","required":false,"type":"int"},{"id":2,"name":"descr","required":false,"type":"string"},{"id":3,"name":"v","required":false,"type":"variant"}]}],"default-spec-id":0,"partition-specs":[{"spec-id":0,"fields":[]}],"last-partition-id":999,"default-sort-order-id":0,"sort-orders":[{"order-id":0,"fields":[]}],"properties":{"write.format.default":"PARQUET"},"current-snapshot-id":8332248857076198293,"next-row-id":0,"refs":{"main":{"snapshot-id":8332248857076198293,"type":"branch"}},"snapshots":[{"sequence-number":1,"snapshot-id":8332248857076198293,"timestamp-ms":1782726955568,"summary":{"operation":"append","trino_query_id":"20260629_095555_00021_wa2kj","trino_user":"trino","changed-partition-count":"0","total-records":"0","total-files-size":"0","total-data-files":"0","total-delete-files":"0","total-position-deletes":"0","total-equality-deletes":"0","engine-version":"481","engine-name":"trino","iceberg-version":"Apache Iceberg 1.10.1 (commit ccb8bc435062171e64bc8b7e5f56e6aed9c5b934)"},"manifest-list":"hdfs://localhost:20500/test-warehouse/trino_variant-8e46befd49c84eceb8028a4558c1f336/metadata/snap-8332248857076198293-1-03e34f74-1a29-4e53-ab2c-6f2c507d534d.avro","schema-id":0,"first-row-id":0,"added-rows":0}],"statistics":[],"partition-statistics":[],"snapshot-log":[{"timestamp-ms":1782726955568,"snapshot-id":8332248857076198293}],"metadata-log":[]} new table data could be mentioned in https://github.com/apache/impala/blob/master/testdata/data/README or another README Another optional idea related to understanding these tables better: it could be useful to format these json files to make them human readable. 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 It would be nice to exercise scenarios that deep copy variants. - adding joins where a variant column is included on build side would materialize it to hash tables - doing the above with PARTITIONED exchange would exercise a different path than simply sending batches to coordinator or BROADCAST exchange (but see my comment in .py file - I am not sure that exchanges are tested at all) - spilling in joins and sorting would check the pointer fixup logic (join and sort do this a bit differently) spilling could be tested by cross joining with a larger table and setting buffer_pool_limit - this could be also nice way to the views as I asked above, e.g. create a view like big_variant_view I don't expect issues with these as variant relies on struct, but also wouldn't rule it out due to the low testing of structs. 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@2461 PS3, Line 2461: self.load_table(unique_database, "trino_variant") A few gaps related to the tests: 1. disable_codegen=true/false seems to be part of the test vector, but batch_size is always the default 0 (so 1024). As the test table is pretty small, all rows will fit to a single batch, so running this with smaller batches too would increase coverage. 2. The test table have a single parquet file - AFAIK this will lead to creating single node plans, leaving exchanges untested. So the test tables should have at least 2 data files. -- 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-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 09 Jul 2026 14:40:49 +0000 Gerrit-HasComments: Yes
