yuqi1129 commented on code in PR #13390:
URL: https://github.com/apache/gravitino/pull/13390#discussion_r4092216031
##########
docs/lakehouse-generic-lance-table.md:
##########
@@ -97,6 +99,38 @@ 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. Blob fields nested in a
+`Struct`, `List`, `Map` or `Union` keep the container as a native Gravitino
type, for example
+`List(External("lance.blob.v2"))`. As with other list columns, the list child
is written back as
+`element`, whatever name it had in the Lance dataset (Lance and pyarrow use
`item`).
+
+### Blob Types
+
+Lance blob columns use a readable external type instead of Arrow JSON:
+
+| External Type Definition | Arrow Field
|
+|----------------------------------------------|---------------------------------------------------------------------------------------------------------|
+| `External("lance.blob.v1")` | `LargeBinary` with metadata
`lance-encoding:blob=true` |
+| `External("lance.blob.v2")` | `Struct<data: LargeBinary,
uri: Utf8>` with metadata `ARROW:extension:name=lance.blob.v2` |
+| `External("lance.blob.v2(with_range=true)")` | `Struct<data: LargeBinary,
uri: Utf8, position: UInt64, size: UInt64>` with the same extension metadata |
+
+`lance.blob.v2` accepts these optional parameters, written as
`lance.blob.v2(key=value, ...)`:
+
+| Parameter | Arrow Field Metadata
| Value |
+|----------------------------|------------------------------------------------|-------------------------------------------------------------|
+| `with_range` | -
| `true` or `false` (default), adds `position` and `size` |
+| `inline_size_threshold` | `lance-encoding:blob-inline-size-threshold`
| Integer >= 0 |
+| `dedicated_size_threshold` | `lance-encoding:blob-dedicated-size-threshold`
| Integer > 0 |
+| `pack_file_size_threshold` | `lance-encoding:blob-pack-file-size-threshold`
| Integer > 0 |
+
+For example, `External("lance.blob.v2(inline_size_threshold=4096,
dedicated_size_threshold=1048576)")`.
+
+Legacy blob columns are rejected by Lance for file version 2.2 and later; use
blob v2 for new tables.
Review Comment:
**Blob v2 cannot be created through the default Gravitino create path.**
Lance 6.0.0 creates new datasets with file version 2.1 by default, and
`LanceTableOperations#createTable` calls `Dataset.write()...execute()` without
`dataStorageVersion`. Result of a real run:
| type | default (=2.1) | 2.1 | 2.2 |
|---|---|---|---|
| `lance.blob.v1` | OK | OK | fails: `Legacy blob columns ... not supported
for file version >= 2.2` |
| `lance.blob.v2` (any params) | fails: `Blob v2 requires file version >=
2.2 (got V2_1)` | fails | OK, round-trips |
So creating a column with `external(lance.blob.v2)` via Gravitino always
fails with a raw Lance `IllegalArgumentException`, and `AddColumn` onto a 2.1
dataset fails the same way, while this line tells users to use blob v2 for new
tables. Could we either expose the data storage version as a table property
(passed to `dataStorageVersion`), or pick 2.2 when the schema contains a blob
v2 field, or at least validate up front with a clear message and document the
constraint? A test in `catalog-lakehouse-generic` that actually creates a Lance
table with blob columns and loads it back would cover this; it fails on the
current head.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/LanceBlobTypes.java:
##########
@@ -0,0 +1,336 @@
+/*
+ * 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 =
Review Comment:
nit: "matching Lance's validation" can't be verified for 6.0.0 for the
inline and pack-file keys (see the comment above). Please point to the Lance
version or source these minimums come from, or drop the claim.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/LanceBlobTypes.java:
##########
@@ -0,0 +1,336 @@
+/*
+ * 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 =
Review Comment:
In the bundled lance-core 6.0.0,
`lance-encoding:blob-dedicated-size-threshold` is the only threshold key
present in the native library (`liblance_jni.*`); `blob-inline-size-threshold`
and `blob-pack-file-size-threshold` do not appear at all. In 6.0.0 the
pack-file threshold is a write option
(`WriteDatasetBuilder#blobPackFileSizeThreshold(long)`), not field metadata. On
a 2.2 dataset the two keys are stored and read back verbatim, but Lance does
not act on them, so users get a silent no-op.
Suggest keeping only `dedicated_size_threshold`, or, if the other two target
newer Lance writers, documenting which Lance version honors them and that they
have no effect on datasets created by the Gravitino server.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/LanceDataTypeConverter.java:
##########
@@ -113,24 +114,25 @@ public Field toArrowField(String name, Type type, boolean
nullable) {
case EXTERNAL:
Types.ExternalType externalType = (Types.ExternalType) type;
+ if (LanceBlobTypes.isBlobCatalogString(externalType.catalogString())) {
+ return LanceBlobTypes.toArrowField(name, nullable,
externalType.catalogString());
Review Comment:
The user's catalog string is stored as written, e.g.
`lance.blob.v2(with_range=false)` or reordered params with extra spaces. On the
next schema refresh (dataset version changed), `toGravitino` replaces it with
the canonical form (`lance.blob.v2`, fixed param order), so the column's type
string changes even though the schema didn't. Consider normalizing on create,
e.g. `LanceBlobTypes.toCatalogString(toArrowField(...))`, so create and load
return the same string.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/LanceDataTypeConverter.java:
##########
@@ -203,7 +205,19 @@ public ArrowType fromGravitino(Type type) {
@Override
public Type toGravitino(Field arrowField) {
+ Optional<String> blobType = LanceBlobTypes.toCatalogString(arrowField);
Review Comment:
Upgrade note: existing Gravitino metadata is only refreshed when the dataset
version changes. Until then, blob v2 columns created before this PR stay a
plain `StructType` (REST describe still returns a struct without the extension
metadata). Legacy blob columns switch from Arrow JSON to
`external(lance.blob.v1)` on the next refresh, which affects any client that
parses that JSON. Worth a line in the docs or release note.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/LanceBlobTypes.java:
##########
@@ -0,0 +1,336 @@
+/*
+ * 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:
`containsKey` treats any `lance-encoding:blob` value as a blob, while
`isCanonicalV1` requires `"true"`. A `Binary` field with
`lance-encoding:blob=false` used to map to `BinaryType`; now it becomes an
Arrow JSON external type. `"true".equals(metadata.get(BLOB_META_KEY))` seems
closer to what Lance means.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/LanceDataTypeConverter.java:
##########
@@ -113,24 +114,25 @@ public Field toArrowField(String name, Type type, boolean
nullable) {
case EXTERNAL:
Types.ExternalType externalType = (Types.ExternalType) type;
+ if (LanceBlobTypes.isBlobCatalogString(externalType.catalogString())) {
+ return LanceBlobTypes.toArrowField(name, nullable,
externalType.catalogString());
+ }
Field field;
try {
field = mapper.readValue(externalType.catalogString(), Field.class);
} catch (Exception e) {
throw new RuntimeException(
"Failed to parse external type catalog string: " +
externalType.catalogString(), e);
}
- Preconditions.checkArgument(
- name.equals(field.getName()),
- "expected field name %s but got %s",
- name,
- field.getName());
Preconditions.checkArgument(
nullable == field.isNullable(),
"expected field nullable %s but got %s",
nullable,
field.isNullable());
- return field;
+ // The column name is authoritative: a renamed column keeps its stored
JSON type.
Review Comment:
This is a real fix: after `RenameColumn`, the stored JSON keeps the old
name, so the old name check made the reverse conversion throw. It's independent
of blobs, though. Could you mention it separately in the PR description (or
split it into its own commit) so it isn't lost in the blob change?
--
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]
