hudi-agent commented on code in PR #19687:
URL: https://github.com/apache/hudi/pull/19687#discussion_r3861198001
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java:
##########
@@ -69,6 +93,11 @@
public class HoodieParquetInputFormat extends HoodieParquetInputFormatBase {
private static final Logger LOG =
LoggerFactory.getLogger(HoodieParquetInputFormat.class);
+ // Compiled once: the raw columns.types screen below runs on every
legacy-path split.
+ private static final Pattern WHITESPACE = Pattern.compile("\\s");
+ // A member a synced variant's Hive type carries, as it appears in a
struct<...> type string:
+ // the cheap screen that decides whether the type string is worth parsing at
all.
+ private static final String HIVE_VARIANT_METADATA =
HoodieSchema.Variant.VARIANT_METADATA_FIELD + ":binary";
Review Comment:
🤖 nit: `HIVE_VARIANT_METADATA` reads like a Hive metadata key or concept,
but its value is a raw substring (`"metadata:binary"`) used only to screen the
`columns.types` string before parsing. Could you rename it to something like
`HIVE_VARIANT_TYPE_SCREEN` or `HIVE_VARIANT_SHAPE_MARKER` to make it clear it's
a quick-filter fragment, not a standalone constant?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/util/SortUtils.java:
##########
@@ -21,14 +21,113 @@
import org.apache.hudi.common.avro.HoodieAvroUtils;
import org.apache.hudi.common.model.HoodieRecord;
import org.apache.hudi.common.schema.HoodieSchema;
+import org.apache.hudi.common.schema.HoodieSchemaField;
+import org.apache.hudi.common.schema.HoodieSchemaType;
import org.apache.hudi.common.util.collection.FlatLists;
+import org.apache.hudi.common.util.collection.Pair;
+import org.apache.hudi.exception.HoodieException;
+import java.util.Locale;
+import java.util.Map;
import java.util.function.Function;
+import java.util.stream.Collectors;
/**
* Utility functions used by BULK_INSERT practitioners while sorting records.
*/
public class SortUtils {
+
+ /**
+ * Rejects sort columns whose type cannot serve as a sort key. Spark's
RowOrdering.isOrderable
+ * is false for both VARIANT and MAP, which is the binding constraint on the
row path. On the
+ * Avro path only MAP is outright uncomparable (GenericData.compare throws
"Can't compare
+ * maps!"); a variant's {metadata, value} record does compare, but by its
bytes, which is never
+ * a meaningful sort key. The walk recurses through records and array
elements just as
+ * isOrderable does, so a struct or an array that merely holds a variant or
a map at depth is
+ * rejected too, and the error names the nested member that made the column
unorderable. Without
+ * this check the failure surfaces deep in the write job (an
AnalysisException from the row
+ * partitioner, a ClassCastException from the record-based one) without
naming the column.
+ *
+ * <p>Matching is case-insensitive, mirroring Spark's column resolution.
Names absent from the
+ * schema (nested paths, meta columns on a data-only schema) are left for
the caller to handle.
+ *
+ * @param sortColumns the configured sort columns, may be null or empty
+ * @param schema schema of the data, with or without metadata fields
+ */
+ public static void validateSortableColumns(String[] sortColumns,
HoodieSchema schema) {
+ if (sortColumns == null || sortColumns.length == 0
+ || schema == null || schema.getType() != HoodieSchemaType.RECORD) {
+ return;
+ }
+ Map<String, HoodieSchemaField> fieldsByLowerName =
schema.getFields().stream()
+ .collect(Collectors.toMap(field ->
field.name().toLowerCase(Locale.ROOT), Function.identity(), (first, second) ->
first));
+ for (String sortColumn : sortColumns) {
+ String columnName = sortColumn.trim();
+ HoodieSchemaField field =
fieldsByLowerName.get(columnName.toLowerCase(Locale.ROOT));
+ if (field == null) {
+ continue;
+ }
+ Option<Pair<String, HoodieSchemaType>> unorderable =
findUnorderableNode(field.schema(), columnName);
+ if (unorderable.isPresent()) {
+ // Only a nested offender needs pointing at; at the top level the
column and its type already say it.
+ String nested = unorderable.get().getLeft().equals(columnName) ? ""
+ : String.format("it holds a %s at '%s', and ",
unorderable.get().getRight(), unorderable.get().getLeft());
+ throw new HoodieException(String.format(
+ "Sorting by column '%s' of type %s is not supported: %sVARIANT and
MAP have no ordering, "
+ + "at any depth. Remove it from the sort columns.",
+ columnName, field.schema().getNonNullType().getType(), nested));
+ }
+ }
+ }
+
+ /**
+ * Mirrors Spark's RowOrdering.isOrderable, but reports where it fails
rather than just that it
+ * does: VARIANT and MAP are the unorderable leaves, a record is orderable
when every field is,
+ * an array when its element type is, and every other type - BLOB (a struct
of atomics in Spark)
+ * and VECTOR (an array of floats) included - is orderable.
+ *
+ * @param schema the node to walk
+ * @param path dotted path of {@code schema}, extended with "." + name per
record field and
+ * with "[]" per array element
+ * @return the path and type of the first unorderable node, or empty when
the schema is orderable
+ */
+ private static Option<Pair<String, HoodieSchemaType>>
findUnorderableNode(HoodieSchema schema, String path) {
+ HoodieSchema unwrapped = schema.isNullable() ? schema.getNonNullType() :
schema;
+ switch (unwrapped.getType()) {
+ case VARIANT:
Review Comment:
🤖 nit: the CSV overload's Javadoc says "Overload for callers holding the
sort columns as a comma-separated string" but doesn't mention that it splits
without trimming. The array overload does trim inside the loop
(`sortColumn.trim()`), so callers get implicit trimming either way — could you
add a one-liner noting that, so the next reader doesn't have to trace the
delegation to confirm it?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
