Copilot commented on code in PR #13390:
URL: https://github.com/apache/gravitino/pull/13390#discussion_r4080749635
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/LanceDataTypeConverter.java:
##########
@@ -203,7 +205,29 @@ public ArrowType fromGravitino(Type type) {
@Override
public Type toGravitino(Field arrowField) {
+ Optional<String> blobType = LanceBlobTypes.toCatalogString(arrowField);
+ if (blobType.isPresent()) {
+ return Types.ExternalType.of(blobType.get());
+ }
+
+ // Only Lance blob metadata is recognized. A blob field outside the
canonical layout keeps the
+ // whole field as Arrow JSON so the blob is not turned into a plain binary
or struct column.
+ if (LanceBlobTypes.isBlob(arrowField)) {
+ return toExternalType(arrowField);
+ }
+
FieldType fieldType = arrowField.getFieldType();
+ // List, map and union children are rebuilt with fixed names and without
metadata, so a blob
+ // below them can only be preserved by keeping the whole subtree as Arrow
JSON. Struct children
+ // keep their names and are converted recursively.
+ ArrowType.ArrowTypeID typeId = fieldType.getType().getTypeID();
+ if ((typeId == ArrowType.ArrowTypeID.List
+ || typeId == ArrowType.ArrowTypeID.Map
+ || typeId == ArrowType.ArrowTypeID.Union)
+ &&
arrowField.getChildren().stream().anyMatch(LanceDataTypeConverter::hasBlobInTree))
{
+ return toExternalType(arrowField);
+ }
Review Comment:
The nested-blob preservation guard only covers `List`, `Map`, and `Union`,
but other list-like container types (notably `LargeList` and `FixedSizeList`)
also rebuild children in Arrow schemas and can similarly lose blob
metadata/names during conversion. This can cause nested blobs under those
containers to be converted recursively and then fail to round-trip correctly.
Consider extending this guard (and any related doc comment) to include
`ArrowTypeID.LargeList` and `ArrowTypeID.FixedSizeList` (and any other
container types that rebuild children) so nested blob subtrees are consistently
preserved as Arrow JSON external types.
##########
docs/lakehouse-generic-lance-table.md:
##########
@@ -97,6 +99,37 @@ For Arrow types not natively mapped in Gravitino, use the
`External(arrow_field_
| `Large List` |
`External("{\"name\":\"col_name\",\"nullable\":true,\"type\":{\"name\":\"largelist\"},\"children\":[{\"name\":\"element\",\"nullable\":true,\"type\":{\"name\":\"int\",\"bitWidth\":32,\"isSigned\":true},\"children\":[]}]}")`
|
| `Fixed-Size List` |
`External("{\"name\":\"col_name\",\"nullable\":true,\"type\":{\"name\":\"fixedsizelist\",\"listSize\":10},\"children\":[{\"name\":\"element\",\"nullable\":true,\"type\":{\"name\":\"int\",\"bitWidth\":32,\"isSigned\":true},\"children\":[]}]}")`
|
+Gravitino types cannot carry Arrow field metadata. When loading a Lance table,
only Lance blob metadata
+is recognized (see [Blob Types](#blob-types)); other field metadata is
ignored. If a blob field appears
+anywhere inside a `List`, `Map` or `Union` field, the whole field is returned
as
+`External(arrow_field_json_str)`.
Review Comment:
This statement calls out `List`, `Map`, and `Union` specifically, but the
type mapping table above includes other list-like containers (e.g., `Large
List`, `Fixed-Size List`). If those containers also rebuild children and thus
require the same 'whole field as Arrow JSON' fallback, the doc should include
them (or explicitly state which Arrow container types are affected) to match
actual converter behavior and reduce surprises.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/LanceDataTypeConverter.java:
##########
@@ -203,7 +205,29 @@ public ArrowType fromGravitino(Type type) {
@Override
public Type toGravitino(Field arrowField) {
+ Optional<String> blobType = LanceBlobTypes.toCatalogString(arrowField);
+ if (blobType.isPresent()) {
+ return Types.ExternalType.of(blobType.get());
+ }
+
+ // Only Lance blob metadata is recognized. A blob field outside the
canonical layout keeps the
+ // whole field as Arrow JSON so the blob is not turned into a plain binary
or struct column.
+ if (LanceBlobTypes.isBlob(arrowField)) {
+ return toExternalType(arrowField);
+ }
+
FieldType fieldType = arrowField.getFieldType();
+ // List, map and union children are rebuilt with fixed names and without
metadata, so a blob
+ // below them can only be preserved by keeping the whole subtree as Arrow
JSON. Struct children
+ // keep their names and are converted recursively.
+ ArrowType.ArrowTypeID typeId = fieldType.getType().getTypeID();
+ if ((typeId == ArrowType.ArrowTypeID.List
+ || typeId == ArrowType.ArrowTypeID.Map
+ || typeId == ArrowType.ArrowTypeID.Union)
+ &&
arrowField.getChildren().stream().anyMatch(LanceDataTypeConverter::hasBlobInTree))
{
+ return toExternalType(arrowField);
+ }
Review Comment:
The new nested-blob preservation behavior is covered for
`List`/`Map`/`Union` in `TestLanceDataTypeConverter`, but if you expand support
to other container types (or if existing code already supports them), this
logic needs explicit tests for those containers too. Add round-trip tests for
at least `LargeList` and `FixedSizeList` containing a blob child (both
canonical and non-canonical) to ensure the intended 'keep whole subtree as
Arrow JSON' behavior and prevent future regressions.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/LanceBlobTypes.java:
##########
@@ -0,0 +1,316 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.lance.common.ops.gravitino;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.arrow.vector.types.pojo.ArrowType;
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.arrow.vector.types.pojo.FieldType;
+
+/**
+ * Converts Lance blob columns between their Arrow field form and a readable
catalog string used by
+ * Gravitino external types.
+ *
+ * <p>Supported catalog strings:
+ *
+ * <ul>
+ * <li>{@code lance.blob.v1}: an Arrow {@code LargeBinary} field with
metadata {@code
+ * lance-encoding:blob=true}.
+ * <li>{@code lance.blob.v2(with_range=true, inline_size_threshold=N,
dedicated_size_threshold=N,
+ * pack_file_size_threshold=N)}: an Arrow struct tagged with {@code
+ * ARROW:extension:name=lance.blob.v2}. All parameters are optional;
without parameters the
+ * parentheses are omitted.
+ * </ul>
+ *
+ * <p>Only Lance blob metadata is recognized; other field metadata is not
represented. A blob field
+ * that does not exactly match the canonical Lance layout is left to the Arrow
JSON representation.
+ */
+final class LanceBlobTypes {
+
+ static final String BLOB_META_KEY = "lance-encoding:blob";
+ static final String ARROW_EXT_NAME_KEY = "ARROW:extension:name";
+ static final String BLOB_V2_EXT_NAME = "lance.blob.v2";
+ static final String INLINE_SIZE_THRESHOLD_META_KEY =
"lance-encoding:blob-inline-size-threshold";
+ static final String DEDICATED_SIZE_THRESHOLD_META_KEY =
+ "lance-encoding:blob-dedicated-size-threshold";
+ static final String PACK_FILE_SIZE_THRESHOLD_META_KEY =
+ "lance-encoding:blob-pack-file-size-threshold";
+
+ static final String V1 = "lance.blob.v1";
+ static final String V2 = "lance.blob.v2";
+
+ private static final String PREFIX = "lance.blob.";
+ private static final String WITH_RANGE = "with_range";
+
+ // Catalog string parameter name -> Arrow metadata key, in canonical output
order.
+ private static final Map<String, String> THRESHOLD_PARAMS =
+ ImmutableMap.of(
+ "inline_size_threshold", INLINE_SIZE_THRESHOLD_META_KEY,
+ "dedicated_size_threshold", DEDICATED_SIZE_THRESHOLD_META_KEY,
+ "pack_file_size_threshold", PACK_FILE_SIZE_THRESHOLD_META_KEY);
+
+ // Catalog string parameter name -> minimum accepted value, matching Lance's
validation.
+ private static final Map<String, Long> THRESHOLD_MINIMUMS =
+ ImmutableMap.of(
+ "inline_size_threshold", 0L,
+ "dedicated_size_threshold", 1L,
+ "pack_file_size_threshold", 1L);
+
+ private static final ArrowType UINT64 = new ArrowType.Int(64, false);
+
+ private static final List<Field> V2_MINIMAL_CHILDREN =
+ ImmutableList.of(
+ nullableChild("data", ArrowType.LargeBinary.INSTANCE),
+ nullableChild("uri", ArrowType.Utf8.INSTANCE));
+
+ private static final List<Field> V2_FULL_CHILDREN =
+ ImmutableList.<Field>builder()
+ .addAll(V2_MINIMAL_CHILDREN)
+ .add(nullableChild("position", UINT64))
+ .add(nullableChild("size", UINT64))
+ .build();
+
+ private static final String SUPPORTED_FORMATS =
+ V1
+ + ", "
+ + V2
+ + "("
+ + WITH_RANGE
+ + "=true, "
+ + String.join("=N, ", THRESHOLD_PARAMS.keySet())
+ + "=N)";
+
+ private LanceBlobTypes() {}
+
+ /**
+ * Returns whether the field carries Lance blob metadata, either legacy blob
or blob v2.
+ *
+ * @param field The Arrow field.
+ * @return true if the field is a Lance blob field.
+ */
+ static boolean isBlob(Field field) {
+ Map<String, String> metadata = field.getMetadata();
+ return metadata != null
+ && (metadata.containsKey(BLOB_META_KEY)
Review Comment:
`isBlob()` treats any field that merely *contains* `lance-encoding:blob` as
a blob, even when the value is not `\"true\"` (e.g., `\"false\"`, `\"yes\"`, or
corrupted values). With the new converter logic, that can change behavior by
forcing such fields (and any parent container detection via `hasBlobInTree`)
into Arrow-JSON external types rather than being treated as normal fields with
ignorable metadata. If the intention is to recognize only actual blob fields,
tighten this to require `\"true\".equals(metadata.get(BLOB_META_KEY))` (while
keeping the v2 extension-name check as-is). If the broader behavior is
intended, it should be explicitly documented as a user-facing behavior change
because it stops ignoring malformed/false blob metadata.
--
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]
