github-actions[bot] commented on code in PR #67921:
URL: https://github.com/apache/doris/pull/67921#discussion_r4002304462
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceTypeConverter.java:
##########
@@ -135,23 +130,23 @@ private static Type toDorisType(Field field, boolean
allowNull) {
case LargeList:
case FixedSizeList:
Review Comment:
[P1] Preserve Null-bearing complex children during recursion
A direct `list<null>` now becomes `ArrayType(Type.NULL)`, but that object's
`isSupported()` is false because `ArrayType.isSupported()` explicitly rejects a
Null item. When this method recurses again, shapes such as `list<list<null>>`
fail the outer check here, and `struct<list<null>>` fails the Struct check
below; `map<string,null>` has the same problem when nested because
`MapType.isSupported()` rejects a Null value. These are valid instances of the
recursive Null-leaf support this change adds, and the BE converter accepts
them, so FE schema discovery instead exposes `UNSUPPORTED`. Please distinguish
the explicit `Type.UNSUPPORTED` conversion result from these newly valid
Null-bearing complex types and add a deeper-nesting case.
##########
docker/thirdparties/docker-compose/iceberg/scripts/lance_build_nested_null.py:
##########
@@ -0,0 +1,61 @@
+# 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.
+
+"""Generate the nested Null regression fixture with pylance 7.0.0 and PyArrow
21.0.0."""
+
+import argparse
+from pathlib import Path
+
+import lance
+import pyarrow as pa
+
+
+def build(output: Path) -> None:
+ null_struct = pa.struct([("empty", pa.null()), ("value", pa.int32())])
+ schema = pa.schema([
+ ("id", pa.int32()),
+ ("null_list", pa.list_(pa.null())),
+ ("null_large_list", pa.large_list(pa.null())),
+ ("null_fixed_list", pa.list_(pa.null(), 2)),
+ ("null_struct", null_struct),
+ ("nested_list", pa.list_(null_struct)),
+ ("null_map", pa.map_(pa.string(), pa.null())),
+ ])
+ rows = [
+ {"id": 1, "null_list": [None, None], "null_large_list": [None],
+ "null_fixed_list": [None, None], "null_struct": {"empty": None,
"value": 10},
+ "nested_list": [{"empty": None, "value": 11}, {"empty": None,
"value": 12}],
+ "null_map": [("a", None), ("b", None)]},
+ {"id": 2, "null_list": None, "null_large_list": None,
"null_fixed_list": None,
+ "null_struct": {"empty": None, "value": 20}, "nested_list": None,
"null_map": None},
+ {"id": 3, "null_list": [], "null_large_list": [],
+ "null_fixed_list": [None, None], "null_struct": {"empty": None,
"value": 30},
+ "nested_list": [], "null_map": []},
+ {"id": 4, "null_list": [None], "null_large_list": [None, None, None],
+ "null_fixed_list": [None, None], "null_struct": {"empty": None,
"value": 40},
+ "nested_list": [{"empty": None, "value": 41}], "null_map": [("c",
None)]},
+ ]
+ # pylance 7 cannot encode a null parent struct with a Null child; BE tests
cover that shape.
+ table = pa.Table.from_pylist(rows, schema=schema)
+ dataset = lance.write_dataset(table, str(output),
data_storage_version="2.2")
Review Comment:
[P2] Keep this fixture in the canonical rebuild
The documented `lance_build_preinstalled_catalog.py` rebuild creates a fresh
staging `lance` directory and then replaces the entire committed fixture tree
with it. That builder neither calls this script nor creates/checks
`nested_null.lance`, so the next normal fixture rebuild silently deletes this
dataset while `test_lance_nested_null.groovy` still reads it from MinIO. Please
integrate this dataset and its validation into the canonical build/check flow
(or otherwise preserve it during the staged replacement).
##########
be/src/format_v2/lance/lance_reader_helper.cpp:
##########
@@ -237,16 +234,16 @@ Status arrow_field_to_doris_type(const
std::shared_ptr<arrow::Field>& field,
case arrow::Type::FIXED_SIZE_LIST: {
const auto list =
std::static_pointer_cast<arrow::BaseListType>(arrow_type);
DataTypePtr value_type;
- RETURN_IF_ERROR(arrow_field_to_doris_type(list->value_field(),
&value_type, false));
+ RETURN_IF_ERROR(arrow_field_to_doris_type(list->value_field(),
&value_type));
*doris_type =
make_nullable(std::make_shared<DataTypeArray>(value_type));
return Status::OK();
}
case arrow::Type::MAP: {
const auto map = std::static_pointer_cast<arrow::MapType>(arrow_type);
DataTypePtr key_type;
DataTypePtr item_type;
Review Comment:
[P1] Reject non-nullable Null children before selecting a physical SerDe
Arrow map keys are non-nullable, so accepting a Null key here is not routed
through the new `DataTypeNullableSerDe` NA branch. The FE preserves that flag
in `MapType.toThrift()`, the BE reconstructs a bare UInt8-backed logical-Null
child, and `DataTypeMapSerDe` sends its Arrow `NullArray` to
`DataTypeNumberSerDe<TYPE_BOOLEAN>`. The failed `BooleanArray` cast is then
dereferenced; with default Arrow validation, even an all-empty `map<null,...>`
crashes. A map value declared non-nullable reaches the same path. Please reject
non-nullable Null children during schema conversion, or otherwise guarantee
safe NA materialization, and cover these positions in FE/BE tests.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
