Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24990 )
Change subject: IMPALA-15460: Read VARIANT object fields stored out of name order ...................................................................... Patch Set 1: (3 comments) I think I get the errors, still processing some parts of the patch. http://gerrit.cloudera.org:8080/#/c/24990/1/be/src/exprs/variant-functions-ir.cc File be/src/exprs/variant-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/24990/1/be/src/exprs/variant-functions-ir.cc@80 PS1, Line 80: NavigatePath Maybe it is a bit hacky, but the change could be made http://gerrit.cloudera.org:8080/#/c/24990/1/be/src/runtime/variant-value.h File be/src/runtime/variant-value.h: http://gerrit.cloudera.org:8080/#/c/24990/1/be/src/runtime/variant-value.h@106 PS1, Line 106: VariantLookupResult Why do we need a new enum instead of using NavStatus? http://gerrit.cloudera.org:8080/#/c/24990/1/be/src/runtime/variant-value.cc File be/src/runtime/variant-value.cc: http://gerrit.cloudera.org:8080/#/c/24990/1/be/src/runtime/variant-value.cc@291 PS1, Line 291: // Compare 'name' with the name of each of this object's field ids instead of looking I agree that the original code is buggy, but what it writes about Spark is against the spec. https://github.com/apache/parquet-format/blob/master/VariantEncoding.md "The field ids and field offsets must be in lexicographical order of the corresponding field names in the metadata dictionary. However, the actual value entries do not need to be in any particular order. This implies that the field_offset values may not be monotonically increasing. " So the original lookup in metadata was indeed wrong, the bisect should happen on the field ids in the struct, with each comparison needing an O(1) lookup in the metadata and then a string compare. "For objects, field IDs and offsets must be listed in the order of the corresponding field names, sorted lexicographically (using unsigned byte ordering for UTF-8)." This is pretty explicit, if Spark writes differently, at least there should be a bug number about it that could be mentioned. It is ok if we need to read things less efficiently due to a Spark/Parquet bug, but this is not how the commit message frames it. -- To view, visit http://gerrit.cloudera.org:8080/24990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ief71d02788a4c830619e6ceaf45ef8eed366411f Gerrit-Change-Number: 24990 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Mon, 05 Oct 2026 18:21:08 +0000 Gerrit-HasComments: Yes
