[Feature](lance) Support additional Arrow and Lance data types - #67325
[Feature](lance) Support additional Arrow and Lance data types#67325wangzhaobo957-cloud wants to merge 7 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Issue Number: close apache#66496 Related PR: apache#65730 Problem Summary: Lance schema discovery rejected Blob v2, Arrow JSON, BFloat16, Arrow null, and Arrow duration fields as unsupported. Add validated FE and BE mappings, materialize Blob payloads through lance-c, normalize BFloat16 values to FLOAT, preserve duration counts as BIGINT, and materialize Arrow null values safely. Support reading Lance Blob v2, Arrow JSON, BFloat16, null, and duration columns. - Test: Unit Test (added; not run locally at user request) - Behavior changed: Yes (the newly supported Lance columns are queryable) - Does this need documentation: Yes (type matrix is documented in the regression suite; user documentation follow-up is required)
Keep the pinned lance-c dependency unchanged. Handle top-level Null safely, reject unsupported nested Null shapes, protect VARBINARY lazy materialization, and update FE, BE, and regression coverage.
eecec32 to
57a5835
Compare
…pache#66496 Reading a Lance Blob v2 column as VARBINARY materialized the full payload, which can reach gigabytes per value and exhaust the BE. lance-c returns the Blob v2 descriptor (kind, position, size, blob_id, blob_uri) by default, so expose that metadata instead of the bytes. - FE LanceTypeConverter maps lance.blob.v2 to STRUCT<kind, position, size, blob_id, blob_uri>. - BE lance_reader validates the descriptor layout, routes Blob v2 through the generic struct mapping, and rejects a materialized LARGE_BINARY payload. - MaterializeProbeVisitor drops the VARBINARY guard now that Blob v2 is a struct covered by hasSubColPath(), restoring plain VARBINARY lazy fetch. - Update FE/BE unit tests and regression suites/outputs accordingly.
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for two issues:
- Correctness / external compatibility: local
local(..., format="lance")schema discovery imports the logical Blob v2 dataset schema, but the new BE validator accepts only the five-field scan descriptor, so a valid Blob dataset is rejected before planning. - Performance: BFloat normalization recursively traverses and allocates Arrow wrapper metadata for every ordinary nested column on every scanner and
take_rowsbatch, even when the subtree contains no BFloat16 extension.
Critical checkpoints:
- Type and reader correctness: reviewed top-level Null, raw-count Duration, Arrow/Lance JSON, nested BFloat16, Blob descriptor materialization, catalog/S3 scans, and vector
take_rows; no other distinct correctness defect remains. The local-TVF Blob boundary above is blocking. - Lifecycle and malformed input: reviewed schema projection, nulls/slices, whole-STRUCT versus subcolumn lazy fetch, row-ID ordering/count, EOF/error cleanup, and invalid extension layouts; no separate lifecycle or malformed-input issue remains.
- Performance and observability: the per-batch no-op normalization is blocking; no separate I/O amplification or profile-counter regression was found.
- Tests: the current BE/unit and catalog/S3 coverage does not exercise local BE schema discovery with Blob v2 or keep the ordinary nested no-extension batch path allocation-free. The inline comments request focused coverage. Tests were not run in this review-only environment.
- Documentation / release note: the live PR description says Blob v2 maps to
VARBINARYand reads a materialized binary payload, while the patch and expected outputs exposeSTRUCT<kind,position,size,blob_id,blob_uri>and explicitly never materialize the payload. Please align the description/release note before merge. - User focus: no additional focus point was supplied.
- Completion: complete and converged after three review rounds on head
5807f583490e9a2fc28f3a5b2e7b11497bfda3a9; all candidates were accepted, deduplicated, or dismissed with evidence.
| }; | ||
| // lance-c returns the Blob v2 descriptor as this fixed 5-field struct (Lance | ||
| // BLOB_V2_DESC_FIELDS): where a Blob lives and how large it is, never its bytes. | ||
| const bool descriptor_layout = fields.size() == 5 && |
There was a problem hiding this comment.
[P1] Keep local TVF schema discovery compatible with logical Blob v2 schemas. fetch_schema() imports lance_dataset_schema() directly, and the pinned lance-c v0.1.7 implementation returns the dataset's logical Blob schema there (struct<data,uri> or the ranged four-field form), not the five-field scan descriptor. Both valid layouts therefore reach this branch and fail fields.size() == 5, so local(..., format="lance") reports the Blob column as unsupported before a query can run; the new catalog/S3 tests use FE schema discovery and do not cover this path. Please map the logical layouts to the advertised descriptor during dataset-schema conversion (while keeping the strict check for scan output), and add a local Lance TVF schema/query regression with Blob v2.
| field->name(), child_fields.size(), child_data.size()); | ||
| } | ||
|
|
||
| auto normalized_data = storage_array->data()->Copy(); |
There was a problem hiding this comment.
[P2] Keep no-op nested normalization out of the batch hot path. _fill_block_from_record_batch() calls this routine for every projected column, and for each ordinary LIST/MAP/STRUCT node with no BFloat16 descendant we still allocate an ArrayData copy and field vector, wrap every child with MakeArray(), and build replacement Fields before changed remains false and all of that work is discarded. This regresses every existing nested Lance scan once per record batch, proportional to schema width/depth, even though the new conversion is needed only for extension-bearing subtrees. Please precompute/gate which fields need normalization, or defer ancestor allocations until a child actually changes, and cover the no-extension path with a wide/deep nested benchmark or allocation-sensitive test.
…he#66496 Separate dataset schema conversion from scan-output validation so local Lance TVFs can map the logical Blob v2 layout to the descriptor struct while scans still require the five-field descriptor. Avoid rebuilding ordinary nested Arrow arrays when their field subtree has no BFloat16 or registered extension array. Add focused schema, fast-path, strict scan validation, fixture scan, and local TVF regression coverage.
|
/review |
3 participants
What problem does this PR solve?
Issue Number: close #66496
Problem Summary:
The Lance reader previously reported several Arrow and Lance-specific types as
UNSUPPORTED, preventing Doris from correctly discovering schemas or reading these columns.This PR adds Doris-side support for:
NULLBIGINTJSONFLOATSTRUCT<kind, position, size, blob_id, blob_uri>The Blob v2 mapping exposes only descriptor metadata. Doris does not materialize or return the Blob binary payload, because a single payload can be several gigabytes.
Implementation details:
struct<data, uri[, position, size]>layout, while scan batches must return the five-field descriptor.LARGE_BINARYBlob payloads and unexpected descriptor layouts.The pinned lance-c dependency already uses
BlobHandling::BlobsDescriptionsby default, so this PR does not change the lance-c dependency.Release note
Add Doris support for Arrow Null and Duration types, Lance JSON and BFloat16 extensions, and Blob v2 descriptor metadata. Blob v2 is exposed as
STRUCT<kind, position, size, blob_id, blob_uri>without loading the binary payload.Check List (For Author)
Coverage includes:
test_lance_catalog_all_typestest_lance_s3_tvftest_lance_local_tvf, covering local TVF schema discovery and Blob descriptor reads.Previously verified results before the latest review fix:
test_lance_catalog_all_types: passedtest_lance_s3_tvf: passedLatest review-fix validation:
Production C++ source passed syntax compilation in normal and
BE_TESTconfigurations.git diff --checkpassed.A complete local BE UT run could not finish because the macOS OpenBLAS
getarchconfiguration process stalled before Doris compilation; CI will run the full suite.Behavior changed:
UNSUPPORTED. Blob v2 now exposes descriptor metadata rather than materializing its payload.Does this need documentation?
Check List (For Reviewer who merge this PR)