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 9: (6 comments) I am getting close to +2ing the change. My remaining concerns are mainly related to avoiding merging something user facing that will need to be changed later (especially if this is merged before Impala 5 is branched). http://gerrit.cloudera.org:8080/#/c/24521/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24521/9//COMMIT_MSG@15 PS9, Line 15: Type system It would be great to add an .md file, e.g. variant-type.md and collect some of the basic info about the type in 10 - 20 lines, focusing on user facing ones, like: - HMS doesn't know about VARIANT, only works for Iceberg - returned to clients as json strings - link to Parquet's variant md + a few examples queries / expressions could be added for future plans My motivation for asking for this comes from trying to find out how Hive returns VARIANTS to clients - two gemini sessions gave me two different answers, and both were wrong. In case of Impala it may find this info in commit messages, but having file with this info is better for both human and AI readers. We have the context for the VARIANT, but it is hard to collect it for someone who is not familiar with it. http://gerrit.cloudera.org:8080/#/c/24521/9//COMMIT_MSG@36 PS9, Line 36: Result display: Opening a new can of worms about how the type is presented to the user. 1. what does DESCRIBE print? 2. what does SHOW CREATE TABLE print? 3. what do Iceberg metadata queries return (for GEOMETRY this revealed an Iceberg lib gap) 4. does impala shell work with variants? the test only use impyla 5. what do TCLIService functions return about the type? is it translated to STRING completely, or the client can differentiate? (GetColumns, GetResultSetMetadata) 6. same for beeswax (note sure how type metadata is returned there) - though probably it would be better to simply reject fetching variants through beeswax For 1-3 adding a test seems simple. 5 may be trickier, as it effects whether JDBC/ODBC will work with VARIANTs. I am ok with moving these client things to another commit (I didn't see a ticket about this in IMPALA-14590), but then it should be written down (e.g. in the .md I proposed above) that the HS2 interface is not final. I would like to avoid releasing something temporary in Impala 5 without communicating it. http://gerrit.cloudera.org:8080/#/c/24521/9//COMMIT_MSG@37 PS9, Line 37: - VARIANT columns are serialized to their JSON representation in query : output (hs2-util, query-result-set), via the backend VariantSlotToJson : helper. It could be mentioned that BINARY fields are base64 encoded (which is a common solution, but not standard AFAIK), similarly to complex types. Trino seems to work the same, not sure about Hive. http://gerrit.cloudera.org:8080/#/c/24521/9//COMMIT_MSG@40 PS9, Line 40: is surfaced as : SQL NULL with a bounded warning log. this was changed http://gerrit.cloudera.org:8080/#/c/24521/9/fe/src/main/jflex/sql-scanner.flex File fe/src/main/jflex/sql-scanner.flex: http://gerrit.cloudera.org:8080/#/c/24521/9/fe/src/main/jflex/sql-scanner.flex@308 PS9, Line 308: keywordMap.put("variant", SqlParserSymbols.KW_VARIANT) Strictly speaking this is a breaking change, as VARIANT was not among the reserved words. As Impala 5 is on the horizon, it would make sense to add expected new keywords in a separate commit (VARIANT, UUID, GEOMETRY, GEOGRAPHY) that will be merbed before Impala 5 for sure. http://gerrit.cloudera.org:8080/#/c/24521/7/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/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant.test@238 PS7, Line 238: must not be incorrectly de-duplicated/merged into a single slot nit: the best is to deduplicate them, and I think that ia happening now -- 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: 9 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: Sat, 25 Jul 2026 11:30:42 +0000 Gerrit-HasComments: Yes
