Skip to content

Commit 027201d

Browse files
Gabriel39Gabriel39
committed
[fix](lance) validate recursive Null schemas and preserve fixture rebuilds
1 parent d9c26bc commit 027201d

6 files changed

Lines changed: 109 additions & 11 deletions

File tree

‎be/src/format_v2/lance/lance_reader_helper.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,11 @@ Status arrow_field_to_doris_type(const std::shared_ptr<arrow::Field>& field,
166166

167167
switch (arrow_type->id()) {
168168
case arrow::Type::NA:
169+
// Required Null leaves can bypass NullableSerDe through FE schema reconstruction.
170+
// Reject them before Arrow NA buffers reach the Boolean-backed physical SerDe.
171+
if (!field->nullable()) {
172+
return Status::NotSupported("non-nullable Lance Arrow Null field: {}", field->name());
173+
}
169174
return nullable_primitive(TYPE_NULL);
170175
case arrow::Type::BOOL:
171176
return nullable_primitive(TYPE_BOOLEAN);

‎be/test/format_v2/lance/lance_nested_null_test.cpp‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,11 +80,15 @@ TEST_F(LanceNestedNullTest, MapsNestedNullSchemas) {
8080
arrow::field("struct", arrow::struct_({null_field})),
8181
arrow::field("map", arrow::map(arrow::utf8(), null_field)),
8282
arrow::field("nested", arrow::list(arrow::struct_({null_field}))),
83+
arrow::field("list_list", arrow::list(arrow::list(null_field))),
84+
arrow::field("struct_list",
85+
arrow::struct_({arrow::field("list", arrow::list(null_field))})),
86+
arrow::field("list_map", arrow::list(arrow::map(arrow::utf8(), null_field))),
8387
});
8488
std::vector<std::string> names;
8589
std::vector<DataTypePtr> types;
8690
ASSERT_TRUE(convert_arrow_schema_to_doris(schema, &names, &types).ok());
87-
ASSERT_EQ(6, types.size());
91+
ASSERT_EQ(9, types.size());
8892
for (const auto& type : types) {
8993
EXPECT_NE(INVALID_TYPE, type->get_primitive_type());
9094
}
@@ -96,6 +100,24 @@ TEST_F(LanceNestedNullTest, MapsNestedNullSchemas) {
96100
}
97101
}
98102

103+
TEST_F(LanceNestedNullTest, RejectsNonNullableNullLeaves) {
104+
const auto leaf = arrow::field("item", arrow::null(), false);
105+
for (const auto& type : std::vector<std::shared_ptr<arrow::DataType>> {
106+
arrow::null(), arrow::list(leaf), arrow::large_list(leaf),
107+
arrow::fixed_size_list(leaf, 2), arrow::struct_({leaf}),
108+
arrow::map(arrow::null(), arrow::utf8()), arrow::map(arrow::utf8(), leaf),
109+
arrow::list(arrow::map(arrow::utf8(), leaf))}) {
110+
SCOPED_TRACE(type->ToString());
111+
std::vector<std::string> names;
112+
std::vector<DataTypePtr> types;
113+
ASSERT_TRUE(convert_arrow_schema_to_doris(
114+
arrow::schema({arrow::field("value", type, false)}), &names, &types)
115+
.ok());
116+
ASSERT_EQ(1, types.size());
117+
EXPECT_EQ(INVALID_TYPE, types[0]->get_primitive_type());
118+
}
119+
}
120+
99121
TEST_F(LanceNestedNullTest, PreservesNullTypeInNestedSchemaRpc) {
100122
const auto null = null_type();
101123
const auto type = make_nullable(std::make_shared<DataTypeStruct>(

‎docker/thirdparties/docker-compose/iceberg/scripts/lance_build_nested_null.py‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
# specific language governing permissions and limitations
1616
# under the License.
1717

18-
"""Generate the nested Null regression fixture with pylance 7.0.0 and PyArrow 21.0.0."""
18+
"""Generate the nested Null regression fixture using lance_fixture_requirements.txt."""
1919

2020
import argparse
2121
from pathlib import Path
@@ -24,7 +24,7 @@
2424
import pyarrow as pa
2525

2626

27-
def build(output: Path) -> None:
27+
def expected_table() -> pa.Table:
2828
null_struct = pa.struct([("empty", pa.null()), ("value", pa.int32())])
2929
schema = pa.schema([
3030
("id", pa.int32()),
@@ -50,9 +50,18 @@ def build(output: Path) -> None:
5050
"nested_list": [{"empty": None, "value": 41}], "null_map": [("c", None)]},
5151
]
5252
# pylance 7 cannot encode a null parent struct with a Null child; BE tests cover that shape.
53-
table = pa.Table.from_pylist(rows, schema=schema)
54-
dataset = lance.write_dataset(table, str(output), data_storage_version="2.2")
55-
assert dataset.to_table().equals(table)
53+
return pa.Table.from_pylist(rows, schema=schema)
54+
55+
56+
def check(output: Path) -> None:
57+
# Validate the persisted schema and values, including empty collections and null parents.
58+
actual = lance.dataset(str(output)).to_table()
59+
assert actual.equals(expected_table()), f"nested Null fixture differs from expected data: {output}"
60+
61+
62+
def build(output: Path) -> None:
63+
lance.write_dataset(expected_table(), str(output), data_storage_version="2.2")
64+
check(output)
5665

5766

5867
if __name__ == "__main__":

‎docker/thirdparties/docker-compose/iceberg/scripts/lance_build_preinstalled_catalog.py‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
The generated catalog contains:
2929
- __manifest Directory Namespace V2 manifest table (with its scalar indexes).
3030
- all_types.lance The pre-existing compatibility-mode root table, re-registered as-is.
31+
- nested_null.lance Nullable Null leaves inside lists, structs, and maps.
3132
- The `doris` namespace with two full-text-search fixtures, one indexed vector table per cell of the
3233
algorithm x element type x metric matrix (hash-prefixed directories), listed in
3334
VECTOR_TABLES below; BREADTH_TABLE, one table carrying the remaining cells at plan
@@ -85,6 +86,7 @@
8586
import lance_namespace
8687
import pyarrow as pa
8788
import pyarrow.ipc as ipc
89+
from lance_build_nested_null import build as build_nested_null, check as check_nested_null
8890
from lance_namespace_urllib3_client.models import (
8991
CreateNamespaceRequest,
9092
CreateTableRequest,
@@ -99,6 +101,7 @@
99101
NUM_PARTITIONS = 4
100102
NAMESPACE = "doris"
101103
ALL_TYPES_DIR = "all_types.lance"
104+
NESTED_NULL_DIR = "nested_null.lance"
102105
MANIFEST_DIR = "__manifest"
103106

104107
# 4-bit PQ keeps codebook training comfortable on 1024 rows. This only serves fixture
@@ -918,7 +921,8 @@ def build_multi_frag(root: Path) -> None:
918921
location = str(root / MULTI_FRAG_DIR)
919922
for index in range(MULTI_FRAG_NUM_FRAGMENTS):
920923
offset = index * MULTI_FRAG_FRAGMENT_ROWS
921-
fragment = make_fragment_table(offset, offset + MULTI_FRAG_FRAGMENT_ROWS)
924+
# The shared builder requires a vector profile even though embedding is dropped below.
925+
fragment = make_fragment_table(COLLINEAR, pa.float32(), offset, offset + MULTI_FRAG_FRAGMENT_ROWS)
922926
fragment = fragment.drop_columns(["embedding"])
923927
# Match all_types.lance (data storage version 2.2) so every committed Lance data file
924928
# shares one on-disk format and the oldest reader (lance-rs 4.0.1) can open it.
@@ -933,6 +937,8 @@ def build_multi_frag(root: Path) -> None:
933937
def build(root: Path, all_types_source: Path) -> None:
934938
shutil.copytree(all_types_source, root / ALL_TYPES_DIR)
935939
build_multi_frag(root)
940+
# Recreate this fixture in staging because promotion replaces the entire catalog tree.
941+
build_nested_null(root / NESTED_NULL_DIR)
936942
namespace = lance_namespace.connect("dir", {"root": str(root)})
937943
namespace.register_table(
938944
RegisterTableRequest(id=["all_types"], location=ALL_TYPES_DIR)
@@ -1721,6 +1727,7 @@ def check_catalog(root: Path) -> None:
17211727
assert nested_path.is_dir(), f"{NESTED_TABLE} location missing: {nested.location}"
17221728
check_nested_dataset(nested.location)
17231729
check_multi_frag(root)
1730+
check_nested_null(root / NESTED_NULL_DIR)
17241731

17251732
full_fts = namespace.describe_table(DescribeTableRequest(id=[NAMESPACE, FTS_TABLE]))
17261733
check_fts_dataset(

‎fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceTypeConverter.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,8 @@ public static Type toDorisType(Field field) {
8484
ArrowType arrowType = field.getType();
8585
switch (arrowType.getTypeID()) {
8686
case Null:
87-
return Type.NULL;
87+
// Required Null leaves cannot use the nullable SerDe that handles Arrow NA buffers.
88+
return field.isNullable() ? Type.NULL : Type.UNSUPPORTED;
8889
case Bool:
8990
return Type.BOOLEAN;
9091
case Int:
@@ -131,7 +132,8 @@ public static Type toDorisType(Field field) {
131132
case FixedSizeList:
132133
requireChildren(field, 1);
133134
Type itemType = toDorisType(field.getChildren().get(0));
134-
return itemType.isSupported() ? new ArrayType(itemType) : Type.UNSUPPORTED;
135+
// Generic isSupported() rejects Null items even inside successfully converted composites.
136+
return itemType.equals(Type.UNSUPPORTED) ? Type.UNSUPPORTED : new ArrayType(itemType);
135137
case Map:
136138
requireChildren(field, 1);
137139
Field entries = field.getChildren().get(0);
@@ -140,14 +142,14 @@ public static Type toDorisType(Field field) {
140142
Field value = entries.getChildren().get(1);
141143
Type keyType = toDorisType(key);
142144
Type valueType = toDorisType(value);
143-
return keyType.isSupported() && valueType.isSupported()
145+
return !keyType.equals(Type.UNSUPPORTED) && !valueType.equals(Type.UNSUPPORTED)
144146
? new MapType(keyType, valueType, key.isNullable(), value.isNullable())
145147
: Type.UNSUPPORTED;
146148
case Struct:
147149
List<StructField> fields = new ArrayList<>();
148150
for (Field child : field.getChildren()) {
149151
Type childType = toDorisType(child);
150-
if (!childType.isSupported()) {
152+
if (childType.equals(Type.UNSUPPORTED)) {
151153
return Type.UNSUPPORTED;
152154
}
153155
fields.add(new StructField(child.getName(), childType,

‎fe/fe-core/src/test/java/org/apache/doris/datasource/lance/LanceTypeConverterTest.java‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,59 @@ public void testNestedNullMappings() {
162162
((StructType) convertedNested.getItemType()).getFields().get(1).getType());
163163
}
164164

165+
@Test
166+
public void testDeepNullComposites() {
167+
Field leaf = Field.nullable("item", ArrowType.Null.INSTANCE);
168+
Field list = new Field("item", FieldType.nullable(ArrowType.List.INSTANCE),
169+
Collections.singletonList(leaf));
170+
Field entries = new Field("entries", FieldType.notNullable(ArrowType.Struct.INSTANCE),
171+
Arrays.asList(Field.notNullable("key", ArrowType.Utf8.INSTANCE), leaf));
172+
Field map = new Field("item", FieldType.nullable(new ArrowType.Map(false)),
173+
Collections.singletonList(entries));
174+
for (Field child : Arrays.asList(list, map)) {
175+
Type childType = LanceTypeConverter.toDorisType(child);
176+
for (ArrowType outer : Arrays.asList(ArrowType.List.INSTANCE,
177+
ArrowType.LargeList.INSTANCE, new ArrowType.FixedSizeList(2))) {
178+
Field nested = new Field("nested", FieldType.nullable(outer),
179+
Collections.singletonList(child));
180+
Assertions.assertEquals(new ArrayType(childType), LanceTypeConverter.toDorisType(nested));
181+
}
182+
Field struct = new Field("nested", FieldType.nullable(ArrowType.Struct.INSTANCE),
183+
Collections.singletonList(child));
184+
Type converted = LanceTypeConverter.toDorisType(struct);
185+
Assertions.assertInstanceOf(StructType.class, converted);
186+
Assertions.assertEquals(childType, ((StructType) converted).getFields().get(0).getType());
187+
Field nestedEntries = new Field("entries", FieldType.notNullable(ArrowType.Struct.INSTANCE),
188+
Arrays.asList(Field.notNullable("key", ArrowType.Utf8.INSTANCE), child));
189+
Field nestedMap = new Field("nested", FieldType.nullable(new ArrowType.Map(false)),
190+
Collections.singletonList(nestedEntries));
191+
Assertions.assertEquals(new MapType(Type.STRING, childType, false, true),
192+
LanceTypeConverter.toDorisType(nestedMap));
193+
}
194+
}
195+
196+
@Test
197+
public void testNonNullableNullLeavesAreUnsupported() {
198+
Field leaf = Field.notNullable("item", ArrowType.Null.INSTANCE);
199+
Assertions.assertEquals(Type.UNSUPPORTED, LanceTypeConverter.toDorisType(leaf));
200+
for (ArrowType outer : Arrays.asList(ArrowType.List.INSTANCE,
201+
ArrowType.LargeList.INSTANCE, new ArrowType.FixedSizeList(2), ArrowType.Struct.INSTANCE)) {
202+
Field nested = new Field("nested", FieldType.nullable(outer), Collections.singletonList(leaf));
203+
Assertions.assertEquals(Type.UNSUPPORTED, LanceTypeConverter.toDorisType(nested));
204+
}
205+
for (boolean nullKey : Arrays.asList(false, true)) {
206+
Field entries = new Field("entries", FieldType.notNullable(ArrowType.Struct.INSTANCE),
207+
Arrays.asList(nullKey ? leaf : Field.notNullable("key", ArrowType.Utf8.INSTANCE),
208+
nullKey ? Field.nullable("value", ArrowType.Utf8.INSTANCE) : leaf));
209+
Field map = new Field("map", FieldType.nullable(new ArrowType.Map(false)),
210+
Collections.singletonList(entries));
211+
Assertions.assertEquals(Type.UNSUPPORTED, LanceTypeConverter.toDorisType(map));
212+
Field nested = new Field("nested", FieldType.nullable(ArrowType.List.INSTANCE),
213+
Collections.singletonList(map));
214+
Assertions.assertEquals(Type.UNSUPPORTED, LanceTypeConverter.toDorisType(nested));
215+
}
216+
}
217+
165218
/** Verifies known extension mappings and storage validation. */
166219
@Test
167220
public void testKnownExtensionMappingsAndStorageValidation() {

0 commit comments

Comments
 (0)